RE: [PATCH] igb: reinit_locked() should be called with rtnl_lock
> -Original Message- > From: Francesco Ruggeri > Sent: Thursday, July 2, 2020 13:20 > To: Kirsher, Jeffrey T > Cc: Nguyen, Anthony L ; Jakub Kicinski > ; David Miller ; open list ker...@vger.kernel.org>; netdev ; intel-wired- > l...@lists.osuosl.org > Subject: Re: [PATCH] igb: reinit_locked() should be called with rtnl_lock > > > > > So will you be sending a v2 of your patch to include the second fix? > > Yes, I am working on it. Just to confirm, v2 should include both fixes, right? Correct.
RE: [PATCH] igb: reinit_locked() should be called with rtnl_lock
> -Original Message-
> From: Francesco Ruggeri
> Sent: Thursday, July 2, 2020 12:35
> To: Kirsher, Jeffrey T
> Cc: Jakub Kicinski ; David Miller ;
> open list ; netdev ;
> intel-wired-...@lists.osuosl.org
> Subject: Re: [PATCH] igb: reinit_locked() should be called with rtnl_lock
>
> > Do not worry about the other Intel drivers, I have our developers looking at
> each of our drivers for the locking issue.
> >
> > @David Miller - I am picking up this patch
>
> There seems to be a second race, independent from the original one, that
> results in a divide error:
>
> kworker reboot -f tx packet
>
> igb_reset_task
> __igb_shutdown
> rtnl_lock()
> ...
> igb_clear_interrupt_scheme
> igb_free_q_vectors
> adapter->num_tx_queues = 0
> ...
> rtnl_unlock()
> rtnl_lock()
> igb_reinit_locked
> igb_down
> igb_up
> netif_tx_start_all_queues
> dev_hard_start_xmit
> igb_xmit_frame
> igb_tx_queue_mapping
> Panics on
> r_idx % adapter->num_tx_queues
>
> Using in igb_reset_task a logic similar to the one in ixgbe_reset_subtask
> (bailing
> if __IGB_DOWN or __IGB_RESETTING is set) seems to avoid the panic.
> That logic was first introduced in ixgbe as part of commit 2f90b8657ec
> ('ixgbe:
> this patch adds support for DCB to the kernel and ixgbe driver').
> Both fixes seem to be needed.
So will you be sending a v2 of your patch to include the second fix?
RE: [PATCH] igb: reinit_locked() should be called with rtnl_lock
> -Original Message-
> From: Francesco Ruggeri
> Sent: Monday, June 29, 2020 21:51
> To: Jakub Kicinski
> Cc: open list ; netdev
> ; intel-wired-...@lists.osuosl.org; David Miller
> ; Kirsher, Jeffrey T
> Subject: Re: [PATCH] igb: reinit_locked() should be called with rtnl_lock
>
> > Would you mind adding a fixes tag here? Probably:
> >
> > Fixes: 9d5c824399de ("igb: PCI-Express 82575 Gigabit Ethernet driver")
>
> That seems to be the commit that introduced the driver in 2.6.25.
> I am not familiar with the history of the driver to tell if this was a day 1
> problem or if it became an issue later.
>
> >
> > And as a matter of fact it looks like e1000e and e1000 have the same
> > bug :/ Would you mind checking all Intel driver producing matches for
> > all the affected ones?
>
> Do you mean identify all Intel drivers that may have the same issue?
>
Do not worry about the other Intel drivers, I have our developers looking at
each of our drivers for the locking issue.
@David Miller - I am picking up this patch
RE: [PATCH v2 01/10] net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()
> -Original Message- > From: Bartosz Golaszewski > Sent: Monday, June 29, 2020 05:04 > To: Kirsher, Jeffrey T ; David S . Miller > ; Jakub Kicinski ; John Crispin > ; Sean Wang ; Mark Lee > ; Matthias Brugger > ; Heiner Kallweit ; Andrew > Lunn ; Florian Fainelli ; Russell King > ; Rob Herring ; Frank Rowand > > Cc: linux-kernel@vger.kernel.org; net...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-media...@lists.infradead.org; > devicet...@vger.kernel.org; Bartosz Golaszewski > > Subject: [PATCH v2 01/10] net: ethernet: ixgbe: check the return value of > ixgbe_mii_bus_init() > > From: Bartosz Golaszewski > > This function may fail. Check its return value and propagate the error code. > > Signed-off-by: Bartosz Golaszewski Acked-by: Jeff Kirsher > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-)
RE: [PATCH v2 02/10] net: ethernet: ixgbe: don't call devm_mdiobus_free()
> -Original Message- > From: Bartosz Golaszewski > Sent: Monday, June 29, 2020 05:04 > To: Kirsher, Jeffrey T ; David S . Miller > ; Jakub Kicinski ; John Crispin > ; Sean Wang ; Mark Lee > ; Matthias Brugger > ; Heiner Kallweit ; Andrew > Lunn ; Florian Fainelli ; Russell King > ; Rob Herring ; Frank Rowand > > Cc: linux-kernel@vger.kernel.org; net...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-media...@lists.infradead.org; > devicet...@vger.kernel.org; Bartosz Golaszewski > > Subject: [PATCH v2 02/10] net: ethernet: ixgbe: don't call > devm_mdiobus_free() > > From: Bartosz Golaszewski > > The idea behind devres is that the release callbacks are called if probe > fails. As > we now check the return value of ixgbe_mii_bus_init(), we can drop the call > devm_mdiobus_free() in error path as the release callback will be called > automatically. > > Signed-off-by: Bartosz Golaszewski Acked-by: Jeff Kirsher > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 14 +++--- > 1 file changed, 3 insertions(+), 11 deletions(-)
RE: [PATCH] e1000e: add ifdef to avoid dead code
> -Original Message-
> From: Eric Dumazet
> On 6/13/20 11:11 PM, Greg Thelen wrote:
> > Commit e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME
> > systems") added e1000e_check_me() but it's only called from
> > CONFIG_PM_SLEEP protected code. Thus builds without CONFIG_PM_SLEEP
> > see:
> > drivers/net/ethernet/intel/e1000e/netdev.c:137:13: warning:
> > 'e1000e_check_me' defined but not used [-Wunused-function]
> >
> > Add CONFIG_PM_SLEEP ifdef guard to avoid dead code.
> >
> > Fixes: e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME
> > systems")
> > Signed-off-by: Greg Thelen
> > ---
> > drivers/net/ethernet/intel/e1000e/netdev.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> Reviewed-by: Eric Dumazet
[Kirsher, Jeffrey T]
This patch is not necessary, after Arnd's patch. Here is his patch:
http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200527134716.948148-1-a...@arndb.de/
and I will be pushing it to Dave's net tree later tonight.
RE: linux-next: build warning after merge of the net-next tree
> -Original Message-
> From: Stephen Rothwell
> Sent: Friday, June 12, 2020 18:16
> To: Kirsher, Jeffrey T
> Cc: David Miller ; Networking
> ; Linux Next Mailing List n...@vger.kernel.org>; Linux Kernel Mailing List ker...@vger.kernel.org>; Lifshits, Vitaly
> Subject: Re: linux-next: build warning after merge of the net-next tree
>
> Hi all,
>
> On Wed, 27 May 2020 01:15:09 + "Kirsher, Jeffrey T"
> wrote:
> >
> > > -Original Message-
> > > From: Stephen Rothwell
> > > Sent: Monday, May 25, 2020 05:40
> > > To: David Miller ; Networking
> > >
> > > Cc: Linux Next Mailing List ; Linux
> > > Kernel Mailing List ; Lifshits, Vitaly
> > > ; Kirsher, Jeffrey T
> > >
> > > Subject: linux-next: build warning after merge of the net-next tree
> > >
> > > Hi all,
> > >
> > > After merging the net-next tree, today's linux-next build (sparc64
> > > defconfig) produced this warning:
> > >
> > > drivers/net/ethernet/intel/e1000e/netdev.c:137:13: warning:
> 'e1000e_check_me'
> > > defined but not used [-Wunused-function] static bool
> > > e1000e_check_me(u16
> > > device_id)
> > > ^~~
> > >
> > > Introduced by commit
> > >
> > > e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME
> > > systems")
> > >
> > > CONFIG_PM_SLEEP is not set for this build.
> > >
> > [Kirsher, Jeffrey T]
> >
> > Vitaly informed me that he has a fix that he will be sending me, I will make
> sure to expedite it.
>
> I am still getting this warning.
I apologize, I have not seen a fix from Vitaly, that I am aware of. I will
make sure you have a patch before Monday.
RE: [PATCH net-next 2/4] ixgbe_ipsec: become aware of when running as a bonding slave
> -Original Message- > From: Jarod Wilson > Sent: Monday, June 8, 2020 14:01 > To: linux-kernel@vger.kernel.org > Cc: Jarod Wilson ; Jay Vosburgh > ; Veaceslav Falico ; Andy > Gospodarek ; David S. Miller ; > Kirsher, Jeffrey T ; Jakub Kicinski > ; Steffen Klassert ; > Herbert Xu ; net...@vger.kernel.org; intel- > wired-...@lists.osuosl.org > Subject: [PATCH net-next 2/4] ixgbe_ipsec: become aware of when running as > a bonding slave > > Slave devices in a bond doing hardware encryption also need to be aware that > they're slaves, so we operate on the slave instead of the bonding master to do > the actual hardware encryption offload bits. > > CC: Jay Vosburgh > CC: Veaceslav Falico > CC: Andy Gospodarek > CC: "David S. Miller" > CC: Jeff Kirsher > CC: Jakub Kicinski > CC: Steffen Klassert > CC: Herbert Xu > CC: net...@vger.kernel.org > CC: intel-wired-...@lists.osuosl.org > Signed-off-by: Jarod Wilson Acked-by: Jeff Kirsher > --- > .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c| 39 +++ > 1 file changed, 31 insertions(+), 8 deletions(-)
RE: [PATCH net-next v3] ice: Replace one-element arrays with flexible-arrays
> -Original Message-
> From: Gustavo A. R. Silva
> Sent: Wednesday, May 27, 2020 07:11
> To: Kirsher, Jeffrey T ; David S. Miller
> ; Jakub Kicinski
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Gustavo A. R. Silva ;
> Kees Cook
> Subject: [PATCH net-next v3] ice: Replace one-element arrays with flexible-
> arrays
>
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
> int length;
> u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as these
> ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning in case
> the flexible array does not occur last in the structure, which will help us
> prevent
> some kind of undefined behavior bugs from being inadvertently introduced[3]
> to the codebase from now on. So, replace the one-element array with a
> flexible-array member.
>
> Also, make use of the offsetof() helper in order to simplify some macros and
> properly calculate the size of the structures that contain flexible-array
> members.
>
> This issue was found with the help of Coccinelle and, audited _manually_.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
[Kirsher, Jeffrey T]
Thanks Gustavo, but we (or I should say Bruce Allan) already has a patch to
resolve this,
and is a bit more thorough. I will make sure you get CC'd on the patch, for
your review.
> ---
> Changes in v3:
> - We still can simply the code even more by using offsetof() just once. :)
>
> Changes in v2:
> - Use offsetof(struct ice_aqc_sw_rules_elem, pdata) instead of
>sizeof(struct ice_aqc_sw_rules_elem) - sizeof(((struct
> ice_aqc_sw_rules_elem
> *)0)->pdata)
> - Update changelog text.
>
> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 6 ++---
> drivers/net/ethernet/intel/ice/ice_switch.c | 23 ++-
> 2 files changed, 10 insertions(+), 19 deletions(-)
>
RE: linux-next: build warning after merge of the net-next tree
> -Original Message-
> From: Stephen Rothwell
> Sent: Monday, May 25, 2020 05:40
> To: David Miller ; Networking
>
> Cc: Linux Next Mailing List ; Linux Kernel Mailing
> List ; Lifshits, Vitaly
> ;
> Kirsher, Jeffrey T
> Subject: linux-next: build warning after merge of the net-next tree
>
> Hi all,
>
> After merging the net-next tree, today's linux-next build (sparc64
> defconfig) produced this warning:
>
> drivers/net/ethernet/intel/e1000e/netdev.c:137:13: warning: 'e1000e_check_me'
> defined but not used [-Wunused-function] static bool e1000e_check_me(u16
> device_id)
> ^~~
>
> Introduced by commit
>
> e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems")
>
> CONFIG_PM_SLEEP is not set for this build.
>
[Kirsher, Jeffrey T]
Vitaly informed me that he has a fix that he will be sending me, I will make
sure to expedite it.
RE: [Intel-wired-lan] [PATCH] Fix typo in the comment
> -Original Message- > From: Intel-wired-lan On Behalf Of Hari > Sent: Friday, May 22, 2020 03:30 > To: da...@davemloft.net; k...@kernel.org > Cc: net...@vger.kernel.org; intel-wired-...@lists.osuosl.org; linux- > ker...@vger.kernel.org; Hari > Subject: [Intel-wired-lan] [PATCH] Fix typo in the comment [Kirsher, Jeffrey T] Please fix the title to "e1000: Fix typo in the comment", other than that your patch looks fine and I will add it to my queue after the title of your patch is fixed. > > Continuous Double "the" in a comment. Changed it to single "the" > > Signed-off-by: Hari > --- > drivers/net/ethernet/intel/e1000/e1000_hw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c > b/drivers/net/ethernet/intel/e1000/e1000_hw.c > index 48428d6a00be..623e516a9630 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c > @@ -3960,7 +3960,7 @@ static s32 e1000_do_read_eeprom(struct e1000_hw > *hw, u16 offset, u16 words, > * @hw: Struct containing variables accessed by shared code > * > * Reads the first 64 16 bit words of the EEPROM and sums the values read. > - * If the the sum of the 64 16 bit words is 0xBABA, the EEPROM's checksum is > + * If the sum of the 64 16 bit words is 0xBABA, the EEPROM's checksum is > * valid. > */ > s32 e1000_validate_eeprom_checksum(struct e1000_hw *hw) > -- > 2.17.1 > > ___ > Intel-wired-lan mailing list > intel-wired-...@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
RE: [PATCH net-next] i40e: Make i40e_shutdown_adminq() return void
> -Original Message- > From: David Miller > Sent: Wednesday, May 6, 2020 14:03 > To: yanai...@huawei.com > Cc: Kirsher, Jeffrey T ; Azarewicz, Piotr > ; intel-wired-...@lists.osuosl.org; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] i40e: Make i40e_shutdown_adminq() return void > > From: Jason Yan > Date: Wed, 6 May 2020 14:18:35 +0800 > > > Fix the following coccicheck warning: > > > > drivers/net/ethernet/intel/i40e/i40e_adminq.c:699:13-21: Unneeded > > variable: "ret_code". Return "0" on line 710 > > > > Signed-off-by: Jason Yan > > Jeff, please pick this up. > > Thank you. [Kirsher, Jeffrey T] Yep already added it to my queue, thanks.
RE: [PATCH net-next] ixgbe: fix signed-integer-overflow warning
> -Original Message- > From: Xie XiuQi > Sent: Monday, May 4, 2020 19:45 > To: Kirsher, Jeffrey T ; da...@davemloft.net > Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: [PATCH net-next] ixgbe: fix signed-integer-overflow warning > > ubsan report this warning, fix it by adding a unsigned suffix. > > UBSAN: signed-integer-overflow in > drivers/net/ethernet/intel/ixgbe/ixgbe_common.c:2246:26 > 65535 * 65537 cannot be represented in type 'int' > CPU: 21 PID: 7 Comm: kworker/u256:0 Not tainted 5.7.0-rc3-debug+ #39 > Hardware name: Huawei TaiShan 2280 V2/BC82AMDC, BIOS 2280-V2 > 03/27/2020 > Workqueue: ixgbe ixgbe_service_task [ixgbe] Call trace: > dump_backtrace+0x0/0x3f0 > show_stack+0x28/0x38 > dump_stack+0x154/0x1e4 > ubsan_epilogue+0x18/0x60 > handle_overflow+0xf8/0x148 > __ubsan_handle_mul_overflow+0x34/0x48 > ixgbe_fc_enable_generic+0x4d0/0x590 [ixgbe] > ixgbe_service_task+0xc20/0x1f78 [ixgbe] > process_one_work+0x8f0/0xf18 > worker_thread+0x430/0x6d0 > kthread+0x218/0x238 > ret_from_fork+0x10/0x18 > > Reported-by: Hulk Robot > Signed-off-by: Xie XiuQi > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Dave, I am picking this up.
RE: [PATCH] igb: Report speed and duplex as unknown when device is runtime suspended
> -Original Message- > From: David Miller > Sent: Monday, May 4, 2020 11:21 > To: kai.heng.f...@canonical.com > Cc: Kirsher, Jeffrey T ; intel-wired- > l...@lists.osuosl.org; net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] igb: Report speed and duplex as unknown when device is > runtime suspended > > From: Kai-Heng Feng > Date: Tue, 5 May 2020 01:32:18 +0800 > > > igb device gets runtime suspended when there's no link partner. We > > can't get correct speed under that state: > > $ cat /sys/class/net/enp3s0/speed > > 1000 > > > > In addition to that, an error can also be spotted in dmesg: > > [ 385.991957] igb :03:00.0 enp3s0: PCIe link lost > > > > Since device can only be runtime suspended when there's no link > > partner, we can directly report the speed and duplex as unknown. > > > > The more generic approach will be wrap get_link_ksettings() with > > begin() and complete() callbacks. However, for this particular issue, > > begin() calls igb_runtime_resume() , which tries to rtnl_lock() while > > the lock is already hold by upper ethtool layer. > > > > So let's take this approach until the igb_runtime_resume() no longer > > needs to hold rtnl_lock. > > > > Suggested-by: Alexander Duyck > > Signed-off-by: Kai-Heng Feng > > I'm assuming I will get this from upstream via Jeff K. [Kirsher, Jeffrey T] Yep, I will be picking this up once Alex's last questions/concerns are addressed.
