Re: [Intel-wired-lan] [PATCH] ice: irdma hardware init failed after suspend/resume
Thank you for your reply. > Sorry for being unclear. I meant, does resuming the system take longer > now? (initcall_debug might give a clue.) I've tested the S3 suspend/resume with the initcall_debug kernel command option, and it shows no clear difference between having or not having the ice_init_rdma in ice_resume: Without ice_init_rdma: ``` [ 104.241129] ice :86:00.0: PM: pci_pm_resume+0x0/0x110 returned 0 after 9415 usecs [ 104.241206] ice :86:00.1: PM: pci_pm_resume+0x0/0x110 returned 0 after 9443 usecs ``` With ice_init_rdma: ``` [ 122.749022] ice :86:00.1: PM: pci_pm_resume+0x0/0x110 returned 0 after 9485 usecs [ 122.749068] ice :86:00.0: PM: pci_pm_resume+0x0/0x110 returned 0 after 9532 usecs ``` > And ice_init_rdma should be moved to ice_rebuild (replace ice_plug_aux_dev) We can defer the ice_init_rdma to the later service task by adopting this. > You should call ice_deinit_rdma in ice_prepare_for_reset (replace > ice_unplug_aux_dev), It seems like we must call ice_deinit_rdma in ice_suspend. If we call it in the later service task, it will: 1. break some existing code setup by ice_resume 2. Since the PCI-X vector table is flushed at the end of ice_suspend, we have no way to release PCI-X vectors for rdma if we had allocated it dynamically The second point is important since we didn't release the PCI-X vectors for rdma (if we allocated it dynamically) in the original ice_suspend, and it's somewhat like a leak in the original code. Best regards, Ricky. On Thu, 30 May 2024 at 04:19, Paul Menzel wrote: > > Dear En-Wei, > > > Thank you for responding so quickly. > > Am 29.05.24 um 05:17 schrieb En-Wei WU: > > […] > > >> What effect does this have on resume time? > > > > When we call ice_init_rdma() at resume time, it will allocate entries > > at pf->irq_tracker.entries and update pf->msix_entries for later use > > (request_irq) by irdma. > > Sorry for being unclear. I meant, does resuming the system take longer > now? (initcall_debug might give a clue.) > > > Kind regards, > > Paul
Re: [Intel-wired-lan] [PATCH] ice: irdma hardware init failed after suspend/resume
Dear En-Wei, Thank you for your quick reply. Am 30.05.24 um 09:08 schrieb En-Wei WU: Sorry for being unclear. I meant, does resuming the system take longer now? (initcall_debug might give a clue.) I've tested the S3 suspend/resume with the initcall_debug kernel command option, and it shows no clear difference between having or not having the ice_init_rdma in ice_resume: Without ice_init_rdma: ``` [ 104.241129] ice :86:00.0: PM: pci_pm_resume+0x0/0x110 returned 0 after 9415 usecs [ 104.241206] ice :86:00.1: PM: pci_pm_resume+0x0/0x110 returned 0 after 9443 usecs ``` With ice_init_rdma: ``` [ 122.749022] ice :86:00.1: PM: pci_pm_resume+0x0/0x110 returned 0 after 9485 usecs [ 122.749068] ice :86:00.0: PM: pci_pm_resume+0x0/0x110 returned 0 after 9532 usecs ``` Thank you for verifying this. Nice to hear, it has no impact. […] Kind regards, Paul
Re: [Intel-wired-lan] REGRESSION: 86167183a17e ("igc: fix a log entry using uninitialized netdev")
On 27.05.24 15:50, Jani Nikula wrote:
>
> Hi all, the Intel graphics CI hits a lockdep issue with commit
> 86167183a17e ("igc: fix a log entry using uninitialized netdev") in
> v6.10-rc1.
FWIW, there is a earlier regression report bisected to that commit id:
https://lore.kernel.org/lkml/cabxgcsokigxafa9tpkjyx7wqjbzqxqk2pztcw-rglfgo8g7...@mail.gmail.com/
And a revert is up for review:
https://lore.kernel.org/all/20240529051307.3094901-1-sasha.nef...@intel.com/
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
Re: [Intel-wired-lan] [iwl-net v1 1/1] igc: Fix Energy Efficient Ethernet support declaration
On 5/22/2024 10:23, Sasha Neftin wrote:
The commit 01cf893bf0f4 ("net: intel: i40e/igc: Remove setting Autoneg in
EEE capabilities") removed SUPPORTED_Autoneg field but left inappropriate
ethtool_keee structure initialization. When "ethtool --show "
(get_eee) invoke, the 'ethtool_keee' structure was accidentally overridden.
Remove the 'ethtool_keee' overriding and add EEE declaration as per IEEE
specification that allows reporting Energy Efficient Ethernet capabilities.
Examples:
Before fix:
ethtool --show-eee enp174s0
EEE settings for enp174s0:
EEE status: not supported
After fix:
EEE settings for enp174s0:
EEE status: disabled
Tx LPI: disabled
Supported EEE link modes: 100baseT/Full
1000baseT/Full
2500baseT/Full
Fixes: 01cf893bf0f4 ("net: intel: i40e/igc: Remove setting Autoneg in EEE
capabilities")
Suggested-by: Dima Ruinskiy
Signed-off-by: Sasha Neftin
---
drivers/net/ethernet/intel/igc/igc_ethtool.c | 9 +++--
drivers/net/ethernet/intel/igc/igc_main.c| 4
2 files changed, 11 insertions(+), 2 deletions(-)
Tested-by: Naama Meir
Re: [Intel-wired-lan] [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
On Thu, May 30, 2024 at 8:52 AM D, Lakshmi Sowjanya
wrote:
> > -Original Message-
> > From: Andy Shevchenko
> > Sent: Monday, May 27, 2024 8:04 PM
> > Mon, May 27, 2024 at 11:48:54AM +, D, Lakshmi Sowjanya kirjoitti:
> > > > -Original Message-
> > > > From: Andy Shevchenko
> > > > Sent: Monday, May 13, 2024 4:49 PM
> > > > On Mon, May 13, 2024 at 04:08:11PM +0530,
> > > > lakshmi.sowjany...@intel.com
> > > > wrote:
...
> > > > > +static ssize_t enable_store(struct device *dev, struct
> > > > > +device_attribute
> > > > *attr, const char *buf,
> > > > > + size_t count)
> > > > > +{
> > > > > + struct pps_tio *tio = dev_get_drvdata(dev);
> > > > > + bool enable;
> > > > > + int err;
> > > >
> > > > (1)
> > > >
> > > > > + err = kstrtobool(buf, &enable);
> > > > > + if (err)
> > > > > + return err;
> > > > > +
> > > > > + guard(spinlock_irqsave)(&tio->lock);
> > > > > + if (enable && !tio->enabled) {
> > > >
> > > > > + if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> > > > > + dev_err(tio->dev, "PPS cannot be started as
> > > > > clock is
> > > > not related
> > > > > +to ART");
> > > >
> > > > Why not simply dev_err(dev, ...)?
> > > >
> > > > > + return -EPERM;
> > > > > + }
> > > >
> > > > I'm wondering if we can move this check to (1) above.
> > > > Because currently it's a good question if we are able to stop PPS
> > > > which was run by somebody else without this check done.
> > >
> > > Do you mean can someone stop the signal without this check?
> > > Yes, this check is not required to stop. So, I feel it need not be moved
> > > to (1).
> > >
> > > Please, correct me if my understanding is wrong.
> >
> > So, there is a possibility to have a PPS being run (by somebody else) even
> > if there
> > is no ART provided?
> >
> > If "yes", your check is wrong to begin with. If "no", my suggestion is
> > correct, i.e.
> > there is no need to stop something that can't be started at all.
>
> It is a "no", PPS doesn't start without ART.
>
> We can move the check to (1), but it would always be checking for ART even in
> case of disable.
Please do,
> Code readability is better with this approach.
>
> struct pps_tio *tio = dev_get_drvdata(dev);
> bool enable;
> int err;
>
> if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> dev_err(dev, "PPS cannot be started as clock is not related
> to ART");
started --> used
> return -EPERM;
> }
>
> err = kstrtobool(buf, &enable);
> if (err)
> return err;
>
> > > > I.o.w. this sounds too weird to me and reading the code doesn't give
> > > > any hint if it's even possible. And if it is, are we supposed to
> > > > touch that since it was definitely *not* us who ran it.
> > >
> > > Yes, we are not restricting on who can stop/start the signal.
> >
> > See above. It's not about this kind of restriction.
> >
> > > > > + pps_tio_direction_output(tio);
> > > > > + hrtimer_start(&tio->timer, first_event(tio),
> > > > HRTIMER_MODE_ABS);
> > > > > + tio->enabled = true;
> > > > > + } else if (!enable && tio->enabled) {
> > > > > + hrtimer_cancel(&tio->timer);
> > > > > + pps_tio_disable(tio);
> > > > > + tio->enabled = false;
> > > > > + }
> > > > > + return count;
> > > > > +}
--
With Best Regards,
Andy Shevchenko
Re: [Intel-wired-lan] [PATCH iwl-next 11/12] idpf: convert header split mode to libeth + napi_build_skb()
Alexander Lobakin wrote: > Currently, idpf uses the following model for the header buffers: > > * buffers are allocated via dma_alloc_coherent(); > * when receiving, napi_alloc_skb() is called and then the header is > copied to the newly allocated linear part. > > This is far from optimal as DMA coherent zone is slow on many systems > and memcpy() neutralizes the idea and benefits of the header split. In the previous revision this assertion was called out, as we have lots of experience with the existing implementation and a previous one based on dynamic allocation one that performed much worse. You would share performance numbers in the next revision https://lore.kernel.org/netdev/0b1cc400-3f58-4b9c-a08b-39104b9f2...@intel.com/T/#me85d509365aba9279275e9b181248247e1f01bb0 This may be so integral to this patch series that asking to back it out now sets back the whole effort. That is not my intent. And I appreciate that in principle there are many potential optizations. But this (OOT) driver is already in use and regressions in existing workloads is a serious headache. As is significant code churn wrt other still OOT feature patch series. This series (of series) modifies the driver significantly, beyond the narrow scope of adding XDP and AF_XDP. > Not > speaking of that XDP can't be run on DMA coherent buffers, but at the > same time the idea of allocating an skb to run XDP program is ill. > Instead, use libeth to create page_pools for the header buffers, allocate > them dynamically and then build an skb via napi_build_skb() around them > with no memory copy. With one exception... > When you enable header split, you except you'll always have a separate > header buffer, so that you could reserve headroom and tailroom only > there and then use full buffers for the data. For example, this is how > TCP zerocopy works -- you have to have the payload aligned to PAGE_SIZE. > The current hardware running idpf does *not* guarantee that you'll > always have headers placed separately. For example, on my setup, even > ICMP packets are written as one piece to the data buffers. You can't > build a valid skb around a data buffer in this case. > To not complicate things and not lose TCP zerocopy etc., when such thing > happens, use the empty header buffer and pull either full frame (if it's > short) or the Ethernet header there and build an skb around it. GRO > layer will pull more from the data buffer later. This W/A will hopefully > be removed one day. > > Signed-off-by: Alexander Lobakin
[Intel-wired-lan] [PATCH net, v2] ice: avoid IRQ collision to fix init failure on ACPI S3 resume
A bug in https://bugzilla.kernel.org/show_bug.cgi?id=218906 describes
that irdma would break and report hardware initialization failed after
suspend/resume with Intel E810 NIC (tested on 6.9.0-rc5).
The problem is caused due to the collision between the irq numbers
requested in irdma and the irq numbers requested in other drivers
after suspend/resume.
The irq numbers used by irdma are derived from ice's ice_pf->msix_entries
which stores mappings between MSI-X index and Linux interrupt number.
It's supposed to be cleaned up when suspend and rebuilt in resume but
it's not, causing irdma using the old irq numbers stored in the old
ice_pf->msix_entries to request_irq() when resume. And eventually
collide with other drivers.
This patch fixes this problem. On suspend, we call ice_deinit_rdma() to
clean up the ice_pf->msix_entries (and free the MSI-X vectors used by
irdma if we've dynamically allocated them). On resume, we call
ice_init_rdma() to rebuild the ice_pf->msix_entries (and allocate the
MSI-X vectors if we would like to dynamically allocate them).
Fixes: f9f5301e7e2d ("ice: Register auxiliary device to provide RDMA")
Tested-by: Cyrus Lien
Signed-off-by: Ricky Wu
---
Changes in v2:
- Change title
- Add Fixes and Tested-by tags
- Fix typo
---
drivers/net/ethernet/intel/ice/ice_main.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index f60c022f7960..ec3cbadaa162 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5544,7 +5544,7 @@ static int ice_suspend(struct device *dev)
*/
disabled = ice_service_task_stop(pf);
- ice_unplug_aux_dev(pf);
+ ice_deinit_rdma(pf);
/* Already suspended?, then there is nothing to do */
if (test_and_set_bit(ICE_SUSPENDED, pf->state)) {
@@ -5624,6 +5624,10 @@ static int ice_resume(struct device *dev)
if (ret)
dev_err(dev, "Cannot restore interrupt scheme: %d\n", ret);
+ ret = ice_init_rdma(pf);
+ if (ret)
+ dev_err(dev, "Reinitialize RDMA during resume failed: %d\n",
ret);
+
clear_bit(ICE_DOWN, pf->state);
/* Now perform PF reset and rebuild */
reset_type = ICE_RESET_PFR;
--
2.43.0
Re: [Intel-wired-lan] [PATCH net-next v6 13/21] bitmap: make bitmap_{get, set}_value8() use bitmap_{read, write}()
On Wed, May 29, 2024 at 04:12:25PM +0100, Robin Murphy wrote:
> Hi Alexander,
>
> On 27/03/2024 3:23 pm, Alexander Lobakin wrote:
> > Now that we have generic bitmap_read() and bitmap_write(), which are
> > inline and try to take care of non-bound-crossing and aligned cases
> > to keep them optimized, collapse bitmap_{get,set}_value8() into
> > simple wrappers around the former ones.
> > bloat-o-meter shows no difference in vmlinux and -2 bytes for
> > gpio-pca953x.ko, which says the optimization didn't suffer due to
> > that change. The converted helpers have the value width embedded
> > and always compile-time constant and that helps a lot.
>
> This change appears to have introduced a build failure for me on arm64
> (with GCC 9.4.0 from Ubuntu 20.04.02) - reverting b44759705f7d makes
> these errors go away again:
>
> In file included from drivers/gpio/gpio-pca953x.c:12:
> drivers/gpio/gpio-pca953x.c: In function ‘pca953x_probe’:
> ./include/linux/bitmap.h:799:17: error: array subscript [1, 1024] is outside
> array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds]
> 799 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> | ^~
> In file included from ./include/linux/atomic.h:5,
> from drivers/gpio/gpio-pca953x.c:11:
> drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’
> 1015 | DECLARE_BITMAP(val, MAX_LINE);
> | ^~~
> ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’
>11 | unsigned long name[BITS_TO_LONGS(bits)]
> |^~~~
> In file included from drivers/gpio/gpio-pca953x.c:12:
> ./include/linux/bitmap.h:800:17: error: array subscript [1, 1024] is outside
> array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds]
> 800 | map[index + 1] |= (value >> space);
> | ~~~^~~
> In file included from ./include/linux/atomic.h:5,
> from drivers/gpio/gpio-pca953x.c:11:
> drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’
> 1015 | DECLARE_BITMAP(val, MAX_LINE);
> | ^~~
> ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’
>11 | unsigned long name[BITS_TO_LONGS(bits)]
> |^~~~
>
> I've not dug further since I don't have any interest in the pca953x
> driver - it just happened to be enabled in my config, so for now I've
> turned it off. However I couldn't obviously see any other reports of
> this, so here it is.
It's a compiler false-positive. The straightforward fix is to disable the
warning
For gcc9+, and it's in Andrew Morton's tree alrady. but there's some discussion
ongoing on how it should be mitigated properlu:
https://lore.kernel.org/all/0ab2702f-8245-4f02-beb7-dcc7d79d5...@app.fastmail.com/T/
Thanks,
YUry
[Intel-wired-lan] [PATCH iwl-net v1] ice: fix 200G link speed message log
Commit 24407a01e57c ("ice: Add 200G speed/phy type use") added support
for 200G PHY speeds, but did not include 200G link speed message
support. As a result the driver incorrectly reports Unknown for 200G
link speed.
Fix this by adding 200G support to ice_print_link_msg().
Fixes: 24407a01e57c ("ice: Add 200G speed/phy type use")
Reviewed-by: Michal Swiatkowski
Signed-off-by: Paul Greenwalt
---
drivers/net/ethernet/intel/ice/ice_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index 1b61ca3a6eb6..899f9a07bfa8 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -805,6 +805,9 @@ void ice_print_link_msg(struct ice_vsi *vsi, bool isup)
}
switch (vsi->port_info->phy.link_info.link_speed) {
+ case ICE_AQ_LINK_SPEED_200GB:
+ speed = "200 G";
+ break;
case ICE_AQ_LINK_SPEED_100GB:
speed = "100 G";
break;
--
2.41.0
Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: fix 200G link speed message log
On 5/30/2024 10:06 AM, Paul Greenwalt wrote:
> Commit 24407a01e57c ("ice: Add 200G speed/phy type use") added support
> for 200G PHY speeds, but did not include 200G link speed message
> support. As a result the driver incorrectly reports Unknown for 200G
> link speed.
>
> Fix this by adding 200G support to ice_print_link_msg().
>
> Fixes: 24407a01e57c ("ice: Add 200G speed/phy type use")
> Reviewed-by: Michal Swiatkowski
> Signed-off-by: Paul Greenwalt
Reviewed-by: Jesse Brandeburg
Re: [Intel-wired-lan] [PATCH net-next v6 13/21] bitmap: make bitmap_{get, set}_value8() use bitmap_{read, write}()
On 30/05/2024 6:11 pm, Yury Norov wrote:
On Wed, May 29, 2024 at 04:12:25PM +0100, Robin Murphy wrote:
Hi Alexander,
On 27/03/2024 3:23 pm, Alexander Lobakin wrote:
Now that we have generic bitmap_read() and bitmap_write(), which are
inline and try to take care of non-bound-crossing and aligned cases
to keep them optimized, collapse bitmap_{get,set}_value8() into
simple wrappers around the former ones.
bloat-o-meter shows no difference in vmlinux and -2 bytes for
gpio-pca953x.ko, which says the optimization didn't suffer due to
that change. The converted helpers have the value width embedded
and always compile-time constant and that helps a lot.
This change appears to have introduced a build failure for me on arm64
(with GCC 9.4.0 from Ubuntu 20.04.02) - reverting b44759705f7d makes
these errors go away again:
In file included from drivers/gpio/gpio-pca953x.c:12:
drivers/gpio/gpio-pca953x.c: In function ‘pca953x_probe’:
./include/linux/bitmap.h:799:17: error: array subscript [1, 1024] is outside
array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds]
799 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
| ^~
In file included from ./include/linux/atomic.h:5,
from drivers/gpio/gpio-pca953x.c:11:
drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’
1015 | DECLARE_BITMAP(val, MAX_LINE);
| ^~~
./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’
11 | unsigned long name[BITS_TO_LONGS(bits)]
|^~~~
In file included from drivers/gpio/gpio-pca953x.c:12:
./include/linux/bitmap.h:800:17: error: array subscript [1, 1024] is outside
array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds]
800 | map[index + 1] |= (value >> space);
| ~~~^~~
In file included from ./include/linux/atomic.h:5,
from drivers/gpio/gpio-pca953x.c:11:
drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’
1015 | DECLARE_BITMAP(val, MAX_LINE);
| ^~~
./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’
11 | unsigned long name[BITS_TO_LONGS(bits)]
|^~~~
I've not dug further since I don't have any interest in the pca953x
driver - it just happened to be enabled in my config, so for now I've
turned it off. However I couldn't obviously see any other reports of
this, so here it is.
It's a compiler false-positive. The straightforward fix is to disable the
warning
For gcc9+, and it's in Andrew Morton's tree alrady. but there's some discussion
ongoing on how it should be mitigated properlu:
https://lore.kernel.org/all/0ab2702f-8245-4f02-beb7-dcc7d79d5...@app.fastmail.com/T/
Ah, great! Guess I really should have scrolled further down my lore
search results - I assumed I was looking for any other reports of a
recent regression in mainline, not ones from 6 months ago :)
Cheers,
Robin.
