Re: [PATCH net v2] i40e: fix the panic when running bpf in xdpdrv mode
Jason Xing wrote: > On Wed, Apr 14, 2021 at 12:27 AM Jesse Brandeburg > wrote: > > > > kerneljasonx...@gmail.com wrote: > > > > > From: Jason Xing > > > > Hi Jason, > > > > Sorry, I missed this on the first time: Added intel-wired-lan, > > please include on any future submissions for Intel drivers. > > get-maintainers script might help here? > > > > Probably I got this wrong in the last email. Did you mean that I should add > intel-wired-lan in the title not the cc list? It seems I should put > this together on > the next submission like this: > > [Intel-wired-lan] [PATCH net v4] Your v3 submittal was correct. My intent was to make sure intel-wired-lan was in CC: If Kuba or Dave wants us to take the fix in via intel-wired-lan trees, then we can do that, or they can apply it directly. I'll ack it on the v3.
Re: [PATCH net v3] i40e: fix the panic when running bpf in xdpdrv mode
kerneljasonx...@gmail.com wrote:
> From: Jason Xing
>
> Fix this panic by adding more rules to calculate the value of @rss_size_max
> which could be used in allocating the queues when bpf is loaded, which,
> however, could cause the failure and then trigger the NULL pointer of
> vsi->rx_rings. Prio to this fix, the machine doesn't care about how many
> cpus are online and then allocates 256 queues on the machine with 32 cpus
> online actually.
>
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
>
> Thus, I attach the key information of the crash-log here.
>
> BUG: unable to handle kernel NULL pointer dereference at
>
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292] ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666] dev_xdp_install+0x4f/0x70
> [2160294.718036] dev_change_xdp_fd+0x11f/0x230
> [2160294.718380] ? dev_disable_lro+0xe0/0xe0
> [2160294.718705] do_setlink+0xac7/0xe70
> [2160294.719035] ? __nla_parse+0xed/0x120
> [2160294.719365] rtnl_newlink+0x73b/0x860
>
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> Co-developed-by: Shujin Li
> Signed-off-by: Shujin Li
> Signed-off-by: Jason Xing
Reviewed-by: Jesse Brandeburg
@Jakub/@DaveM - feel free to apply this directly.
Re: [Patch v4 1/3] lib: Restrict cpumask_local_spread to houskeeping CPUs
Nitesh Narayan Lal wrote: > > The original issue as seen, was that if you rmmod/insmod a driver > > *without* irqbalance running, the default irq mask is -1, which means > > any CPU. The older kernels (this issue was patched in 2014) used to use > > that affinity mask, but the value programmed into all the interrupt > > registers "actual affinity" would end up delivering all interrupts to > > CPU0, > > So does that mean the affinity mask for the IRQs was different wrt where > the IRQs were actually delivered? > Or, the affinity mask itself for the IRQs after rmmod, insmod was changed > to 0 instead of -1? The smp_affinity was 0xfff, and the kernel chooses which interrupt to place the interrupt on, among any of the bits set. > I did a quick test on top of 5.12.0-rc6 by comparing the i40e IRQ affinity > mask before removing the kernel module and after doing rmmod+insmod > and didn't find any difference. with the patch in question removed? Sorry, I'm confused what you tried. > > > and if the machine was under traffic load incoming when the > > driver loaded, CPU0 would start to poll among all the different netdev > > queues, all on CPU0. > > > > The above then leads to the condition that the device is stuck polling > > even if the affinity gets updated from user space, and the polling will > > continue until traffic stops. > > > >> The problem with the commit is that when we overwrite the affinity mask > >> based on the hinting mask we completely ignore the default SMP affinity > >> mask. If we do want to overwrite the affinity based on the hint mask we > >> should atleast consider the default SMP affinity. > > For the issue where the IRQs don't follow the default_smp_affinity mask > because of this patch, the following are the steps by which it can be easily > reproduced with the latest linux kernel: > > # Kernel > 5.12.0-rc6+ > As we can see in the above trace the initial affinity for the IRQ 1478 was > correctly set as per the default_smp_affinity mask which includes CPU 42, > however, later on, it is updated with CPU3 which is returned from > cpumask_local_spread(). > > > Maybe the right thing is to fix which CPUs are passed in as the valid > > mask, or make sure the kernel cross checks that what the driver asks > > for is a "valid CPU"? > > > > Sure, if we can still reproduce the problem that your patch was fixing then > maybe we can consider adding a new API like cpumask_local_spread_irq in > which we should consider deafult_smp_affinity mask as well before returning > the CPU. I'm sure I don't have a reproducer of the original problem any more, it is lost somewhere 8 years ago. I'd like to be able to repro the original issue, but I can't. Your description of the problem makes it obvious there is an issue. It appears as if cpumask_local_spread() is the wrong function to use here. If you have any suggestions please let me know. We had one other report of this problem as well (I'm not sure if it's the same as your report) https://lkml.org/lkml/2021/3/28/206 https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20210125/023120.html
Re: [PATCH net v2] i40e: fix the panic when running bpf in xdpdrv mode
kerneljasonx...@gmail.com wrote:
> From: Jason Xing
Hi Jason,
Sorry, I missed this on the first time: Added intel-wired-lan,
please include on any future submissions for Intel drivers.
get-maintainers script might help here?
>
> Fix this panic by adding more rules to calculate the value of @rss_size_max
> which could be used in allocating the queues when bpf is loaded, which,
> however, could cause the failure and then trigger the NULL pointer of
> vsi->rx_rings. Prio to this fix, the machine doesn't care about how many
> cpus are online and then allocates 256 queues on the machine with 32 cpus
> online actually.
>
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
>
> Thus, I attach the key information of the crash-log here.
>
> BUG: unable to handle kernel NULL pointer dereference at
>
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292] ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666] dev_xdp_install+0x4f/0x70
> [2160294.718036] dev_change_xdp_fd+0x11f/0x230
> [2160294.718380] ? dev_disable_lro+0xe0/0xe0
> [2160294.718705] do_setlink+0xac7/0xe70
> [2160294.719035] ? __nla_parse+0xed/0x120
> [2160294.719365] rtnl_newlink+0x73b/0x860
>
> Fixes: 41c445ff0f48 ("i40e: main driver core")
>
This Fixes line should be connected to the Sign offs with
no linefeeds between.
> Signed-off-by: Jason Xing
> Signed-off-by: Shujin Li
Did Shujin contribute to this patch? Why are they signing off? If
they developed this patch with you, it should say:
Co-developed-by: Shujin
Signed-off-by: Shujin ...
Signed-off-by: Jason ...
Your signature should be last if you sent the patch. The sign-offs are
like a chain of custody, please review
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Thanks,
Jesse
Re: [PATCH] i40e: fix the panic when running bpf in xdpdrv mode
kerneljasonx...@gmail.com wrote:
> From: Jason Xing
>
> Re: [PATCH] i40e: fix the panic when running bpf in xdpdrv mode
Please use netdev style subject lines when patching net kernel to
indicate which kernel tree this is targeted at, "net" or "net-next"
[PATCH net v2] i40e: ...
> Fix this by add more rules to calculate the value of @rss_size_max which
Fix this panic by adding ...
> could be used in allocating the queues when bpf is loaded, which, however,
> could cause the failure and then triger the NULL pointer of vsi->rx_rings.
trigger
> Prio to this fix, the machine doesn't care about how many cpus are online
> and then allocates 256 queues on the machine with 32 cpus online
> actually.
>
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
>
> Thus, I attach the key information of the crash-log here.
>
> BUG: unable to handle kernel NULL pointer dereference at
>
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292] ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666] dev_xdp_install+0x4f/0x70
> [2160294.718036] dev_change_xdp_fd+0x11f/0x230
> [2160294.718380] ? dev_disable_lro+0xe0/0xe0
> [2160294.718705] do_setlink+0xac7/0xe70
> [2160294.719035] ? __nla_parse+0xed/0x120
> [2160294.719365] rtnl_newlink+0x73b/0x860
>
> Signed-off-by: Jason Xing
> Signed-off-by: Shujin Li
if you send to "net" - I suspect you should supply a Fixes: line, above
the sign-offs.
In this case however, this bug has been here since the beginning of the
driver, but the patch will easily apply, so please supply
Fixes: 41c445ff0f48 ("i40e: main driver core")
> ---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 521ea9d..4e9a247 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -11867,6 +11867,7 @@ static int i40e_sw_init(struct i40e_pf *pf)
> {
> int err = 0;
> int size;
> + u16 pow;
>
> /* Set default capability flags */
> pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
> @@ -11885,6 +11886,11 @@ static int i40e_sw_init(struct i40e_pf *pf)
> pf->rss_table_size = pf->hw.func_caps.rss_table_size;
> pf->rss_size_max = min_t(int, pf->rss_size_max,
>pf->hw.func_caps.num_tx_qp);
> +
> + /* find the next higher power-of-2 of num cpus */
> + pow = roundup_pow_of_two(num_online_cpus());
> + pf->rss_size_max = min_t(int, pf->rss_size_max, pow);
> +
The fix itself is fine, and is correct as far as I can tell, thank you
for sending the patch!
> if (pf->hw.func_caps.rss) {
> pf->flags |= I40E_FLAG_RSS_ENABLED;
> pf->alloc_rss_size = min_t(int, pf->rss_size_max,
Re: [PATCH] net: ethernet: intel: Fix a typo in the file ixgbe_dcb_nl.c
Bhaskar Chowdhury wrote: > > s/Reprogam/Reprogram/ > > Signed-off-by: Bhaskar Chowdhury Reviewed-by: Jesse Brandeburg
Re: [net-next PATCH v2 00/10] ethtool: Factor out common code related to writing ethtool strings
Alexander Duyck wrote: > This patch set is meant to be a cleanup and refactoring of common code bits > from several drivers. Specificlly a number of drivers engage in a pattern > where they will use some variant on an sprintf or memcpy to write a string > into the ethtool string array and then they will increment their pointer by > ETH_GSTRING_LEN. > > Instead of having each driver implement this independently I am refactoring > the code so that we have one central function, ethtool_sprintf that does > all this and takes a double pointer to access the data, a formatted string > to print, and the variable arguments that are associated with the string. > > Changes from v1: > Fixed usage of char ** vs unsigned char ** in hisilicon drivers > > Changes from RFC: > Renamed ethtool_gsprintf to ethtool_sprintf > Fixed reverse xmas tree issue in patch 2 > Thanks Alex, I had a look over the whole thing and it looks good to me. Reviewed-by: Jesse Brandeburg
Re: [net-next PATCH v2 02/10] intel: Update drivers to use ethtool_sprintf
Alexander Duyck wrote: > From: Alexander Duyck > > Update the Intel drivers to make use of ethtool_sprintf. The general idea > is to reduce code size and overhead by replacing the repeated pattern of > string printf statements and ETH_STRING_LEN counter increments. > > Signed-off-by: Alexander Duyck Thanks! Acked-by: Jesse Brandeburg
Re: [PATCH] iavf: fix locking of critical sections
Jakub Kicinski wrote: > > > I personally think that the overuse of flags in Intel drivers brings > > > nothing but trouble. At which point does it make sense to just add a > > > lock / semaphore here rather than open code all this with no clear > > > semantics? No code seems to just test the __IAVF_IN_CRITICAL_TASK flag, > > > all the uses look like poor man's locking at a quick grep. What am I > > > missing? > > > > I agree with you that the locking could be done with other locking > > mechanisms just as good. I didn't invent the current method so I'll let > > Intel comment on that part, but I'd like to point out that what I'm > > making use of is fixing what is currently in the driver. > > Right, I should have made it clear that I don't blame you for the > current state of things. Would you mind sending a patch on top of > this one to do a conversion to a semaphore? > > Intel folks any opinions? I know Slawomir has been working closely with Stefan on figuring out the right ways to fix this code. Hopefully he can speak for himself, but I know he's on Europe time. As for conversion to mutexes I'm a big fan, and as long as we don't have too many collisions with the RTNL lock I think it's a reasonable improvement to do, and if Stefan doesn't want to work on it, we can look into whether Slawomir or his team can.
Re: [RFC PATCH 01/10] ethtool: Add common function for filling out strings
Jakub Kicinski wrote:
> On Wed, 10 Mar 2021 17:35:17 -0800 Alexander Duyck wrote:
> > From: Alexander Duyck
> > +/**
> > + * ethtool_gsprintf - Write formatted string to ethtool string data
> > + * @data: Pointer to start of string to update
> > + * @fmt: Format of string to write
> > + *
> > + * Write formatted string to data. Update data to point at start of
> > + * next string.
> > + */
> > +extern __printf(2, 3) void ethtool_gsprintf(u8 **data, const char *fmt,
> > ...);
>
> I'd drop the 'g' TBH, it seems to have made its way from the ethtool
> command ('gstrings') to various places but without the 'string' after
> it - it becomes less and less meaningful. Just ethtool_sprintf() would
> be fine IMHO.
>
> Other than that there is a minor rev xmas tree violation in patch 2 :)
I agree with Jakub, and I really like this series, it's a good clean up.
I'll be glad to add a reviewed by tag to v2.
Re: [RFC PATCH 02/10] intel: Update drivers to use ethtool_gsprintf
Alexander Duyck wrote: > From: Alexander Duyck > > Update the Intel drivers to make use of ethtool_gsprintf. The general idea > is to reduce code size and overhead by replacing the repeated pattern of > string printf statements and ETH_STRING_LEN counter increments. > > Signed-off-by: Alexander Duyck > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 16 ++ > drivers/net/ethernet/intel/ice/ice_ethtool.c | 55 > +++--- > drivers/net/ethernet/intel/igb/igb_ethtool.c | 40 ++-- > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 40 ++-- > 4 files changed, 50 insertions(+), 101 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index c70dec65a572..932c6635cfd6 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -2368,21 +2368,15 @@ static void i40e_get_priv_flag_strings(struct > net_device *netdev, u8 *data) > struct i40e_netdev_priv *np = netdev_priv(netdev); > struct i40e_vsi *vsi = np->vsi; > struct i40e_pf *pf = vsi->back; > - char *p = (char *)data; > + u8 *p = data; > unsigned int i; As Jakub said, RCT... :-) no other comments on the rest of the patch, looks good and Thanks!
Re: [RFC net-next] iavf: refactor plan proposal
Jakub Kicinski wrote: > On Mon, 8 Mar 2021 16:28:58 -0800 Jesse Brandeburg wrote: > > Hello, > > > > We plan to refactor the iavf module and would appreciate community and > > maintainer feedback on our plans. We want to do this to realize the > > usefulness of the common code module for multiple drivers. This > > proposal aims to avoid disrupting current users. > > > > The steps we plan are something like: > > 1) Continue upstreaming of the iecm module (common module) and > >the initial feature set for the idpf driver[1] utilizing iecm. > > Oh, that's still going? there wasn't any revision for such a long time > I deleted my notes :-o Argh! sorry about the delay. These proposed driver changes impacted progress on this patch series, we should have done a better job communicating what was going on. > > We are looking to make sure that the mode of our refactoring will meet > > the community's expectations. Any advice or feedback is appreciated. > > Sounds like a slow, drawn out process painful to everyone involved. > > The driver is upstream. My humble preference is that Intel sends small > logical changes we can review, and preserve a meaningful git history. We are attempting to make it as painless and quick as possible. With that said, I see your point and am driving some internal discussions to see what we can do differently. The primary reason for the plan proposed is the code reuse model we've chosen. With the change to the common module, the new iavf is significantly different and replacing the old avf base with the new would take many unnecessary intermediate steps that would be thrown away at the end. The end design will use the code from the common module with hooks to get device specific implementation where necessary. After putting in place the new-avf code we can update the iavf with new functionality which is already present in the common module. Thanks, Jesse
Re: [RFC net-next] iavf: refactor plan proposal
Leon Romanovsky wrote: > > 3) Plan is to make the "new" iavf driver the default iavf once > >extensive regression testing can be completed. > > a. Current proposal is to make CONFIG_IAVF have a sub-option > >CONFIG_IAVF_V2 that lets the user adopt the new code, > >without changing the config for existing users or breaking > >them. > > I don't think that .config options are considered ABIs, so it is unclear > what do you mean by saying "disrupting current users". Instead of the > complication wrote above, do like any other driver does: perform your > testing, submit the code and switch to the new code at the same time. Because this VF driver runs on multiple hardware PFs (they all expose the same VF device ID) the testing matrix is quite huge and will take us a while to get through it. We aim to avoid making users's life hard by having CONFIG_IAVF=m become a surprise new code base behind the back of the user. I've always thought that the .config options *are* a sort of ABI, because when you do "make oldconfig" it tries to pick up your previous configuration and if, for instance, a driver changes it's Kconfig name, it will not pick up the old value of the old driver Kconfig name for the new build, and with either default or ask the user. The way we're proposing I think will allow the old driver to stay default until the user answers Y to the "new option" for the new, iecm based code. > > [1] > > https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.ngu...@intel.com/ > > Please don't introduce module parameters in new code. Thanks, we certainly won't. :-) I'm not sure why you commented about module parameters, but the above link is to the previous submission for a new driver that uses some common code as a module (iecm) for a new device driver (idpf) we had sent. The point of this email was to solicit feedback and give notice about doing a complicated refactor/replace where we end up re-using iecm for the new version of the iavf code, with the intent to be up front and working with the community throughout the process. Because of the complexity, we want do the right thing the first time so we can to avoid a restart/redesign. Thanks, Jesse
[RFC net-next] iavf: refactor plan proposal
Hello, We plan to refactor the iavf module and would appreciate community and maintainer feedback on our plans. We want to do this to realize the usefulness of the common code module for multiple drivers. This proposal aims to avoid disrupting current users. The steps we plan are something like: 1) Continue upstreaming of the iecm module (common module) and the initial feature set for the idpf driver[1] utilizing iecm. 2) Introduce the refactored iavf code as a "new" iavf driver with the same device ID, but Kconfig default to =n to enable testing. a. Make this exclusive so if someone opts in to "new" iavf, then it disables the original iavf (?) b. If we do make it exclusive in Kconfig can we use the same name? 3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed. a. Current proposal is to make CONFIG_IAVF have a sub-option CONFIG_IAVF_V2 that lets the user adopt the new code, without changing the config for existing users or breaking them. We are looking to make sure that the mode of our refactoring will meet the community's expectations. Any advice or feedback is appreciated. Thanks, Jesse, Alice, Alan [1] https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.ngu...@intel.com/
Re: [Intel-wired-lan] [PATCH intel-net 0/3] intel: Rx headroom fixes
Maciej Fijalkowski wrote: > Fix Rx headroom by calling *_rx_offset() after the build_skb Rx ring > flag is set. > > It was reported by Jesper in [0] that XDP_REDIRECT stopped working after > [1] patch in i40e. Looks good to me, thanks for the fixes Maciej, and especially to Jesper for the report of the issue. For the series: Reviewed-by: Jesse Brandeburg
Re: [PATCH] igb: unbreak I2C bit-banging on i350
Jan Kundrát wrote: > The driver tried to use Linux' native software I2C bus master > (i2c-algo-bits) for exporting the I2C interface that talks to the SFP > cage(s) towards userspace. As-is, however, the physical SCL/SDA pins > were not moving at all, staying at logical 1 all the time. > > The main culprit was the I2CPARAMS register where igb was not setting > the I2CBB_EN bit. That meant that all the careful signal bit-banging was > actually not being propagated to the chip pads (I verified this with a > scope). > > The bit-banging was not correct either, because I2C is supposed to be an > open-collector bus, and the code was driving both lines via a totem > pole. The code was also trying to do operations which did not make any > sense with the i2c-algo-bits, namely manipulating both SDA and SCL from > igb_set_i2c_data (which is only supposed to set SDA). I'm not sure if > that was meant as an optimization, or was just flat out wrong, but given > that the i2c-algo-bits is set up to work with a totally dumb GPIO-ish > implementation underneath, there's no need for this code to be smart. > > The open-drain vs. totem-pole is fixed by the usual trick where the > logical zero is implemented via regular output mode and outputting a > logical 0, and the logical high is implemented via the IO pad configured > as an input (thus floating), and letting the mandatory pull-up resistors > do the rest. Anything else is actually wrong on I2C where all devices > are supposed to have open-drain connection to the bus. > > The missing I2CBB_EN is set (along with a safe initial value of the > GPIOs) just before registering this software I2C bus. > > The chip datasheet mentions HW-implemented I2C transactions (SFP EEPROM > reads and writes) as well, but I'm not touching these for simplicity. > > Tested on a LR-Link LRES2203PF-2SFP (which is an almost-miniPCIe form > factor card, a cable, and a module with two SFP cages). There was one > casualty, an old broken SFP we had laying around, which was used to > solder some thin wires as a DIY I2C breakout. Thanks for your service. > With this patch in place, I can `i2cdump -y 3 0x51 c` and read back data > which make sense. Yay. > > Signed-off-by: Jan Kundrát > See-also: https://www.spinics.net/lists/netdev/msg490554.html Thanks for the patch! I'd like someone on our team to double check things are working still on some of the stock Intel boards, but the code changes look fine. Reviewed-by: Jesse Brandeburg
Re: [PATCH net 2/2] igb: Fix duplicate include guard
Tom Seewald wrote:
> The include guard "_E1000_HW_H_" is used by two separate header files in
> two different drivers (e1000/e1000_hw.h and igb/e1000_hw.h). Using the
> same include guard macro in more than one header file may cause
> unexpected behavior from the compiler. Fix this by renaming the
> duplicate guard in the igb driver.
>
> Fixes: 9d5c824399de ("igb: PCI-Express 82575 Gigabit Ethernet driver")
> Signed-off-by: Tom Seewald
Change is simple and makes sense.
Reviewed-by: Jesse Brandeburg
Re: [PATCH net] net: enetc: initialize the RFS and RSS memories
Vladimir Oltean wrote:
> Discussion with the hardware design engineers reveals that on LS1028A,
> the hardware does not do initialization of that RFS/RSS memory, and that
> software should clear/initialize the entire table before starting to
> operate. That comes as a bit of a surprise, since the driver does not do
> initialization of the RFS memory. Also, the initialization of the
> Receive Side Scaling is done only partially.
...
> Reported-by: Michael Walle
> Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet
> drivers")
> Signed-off-by: Vladimir Oltean
Reviewed-by: Jesse Brandeburg
Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
Song Yoong Siang wrote:
> From: Mohammad Athari Bin Ismail
>
> When disable CBS, mode_to_use parameter is not updated even the operation
> mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
> when tc_setup_cbs() function is called to re-enable CBS, the operation
> mode of Tx Queue remains at DCB, which causing CBS fails to work.
>
> This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
> after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
> callback function.
>
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Suggested-by: Gomes, Vinicius
> Signed-off-by: Mohammad Athari Bin Ismail
> Signed-off-by: Song, Yoong Siang
Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next 1/9] lan78xx: add NAPI interface support
NB: I thought I'd have a close look at this since I thought I
understand NAPI pretty well, but using NAPI to transmit frames as well
as with a usb device has got me pretty confused. Also, I suspect that
you didn't try compiling this against the net-next kernel.
I'm stopping my review only partially completed, please address issues
https://patchwork.kernel.org/project/netdevbpf/patch/20210204113121.29786-2-john.efstathia...@pebblebay.com/
It might make it easier for reviewers to split the "infrastructure"
refactors this patch uses into separate pieces. I know it is more work
and this is tested already by you, but this is a pretty complicated
chunk of code to review.
John Efstathiades wrote:
> Improve driver throughput and reduce CPU overhead by using the NAPI
> interface for processing Rx packets and scheduling Tx and Rx URBs.
>
> Provide statically-allocated URB and buffer pool for both Tx and Rx
> packets to give greater control over resource allocation.
>
> Remove modification of hard_header_len that prevents correct operation
> of generic receive offload (GRO) handling of TCP connections.
>
> Signed-off-by: John Efstathiades
> ---
> drivers/net/usb/lan78xx.c | 1176 -
> 1 file changed, 775 insertions(+), 401 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index e81c5699c952..1c872edc816a 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -47,17 +47,17 @@
>
> #define MAX_RX_FIFO_SIZE (12 * 1024)
> #define MAX_TX_FIFO_SIZE (12 * 1024)
> -#define DEFAULT_BURST_CAP_SIZE (MAX_TX_FIFO_SIZE)
> -#define DEFAULT_BULK_IN_DELAY(0x0800)
> #define MAX_SINGLE_PACKET_SIZE (9000)
> #define DEFAULT_TX_CSUM_ENABLE (true)
> #define DEFAULT_RX_CSUM_ENABLE (true)
> #define DEFAULT_TSO_CSUM_ENABLE (true)
> #define DEFAULT_VLAN_FILTER_ENABLE (true)
> #define DEFAULT_VLAN_RX_OFFLOAD (true)
> -#define TX_OVERHEAD (8)
> +#define TX_ALIGNMENT (4)
> #define RXW_PADDING 2
>
> +#define MIN_IPV4_DGRAM 68
> +
> #define LAN78XX_USB_VENDOR_ID(0x0424)
> #define LAN7800_USB_PRODUCT_ID (0x7800)
> #define LAN7850_USB_PRODUCT_ID (0x7850)
> @@ -78,6 +78,44 @@
>WAKE_MCAST | WAKE_BCAST | \
>WAKE_ARP | WAKE_MAGIC)
>
> +#define LAN78XX_NAPI_WEIGHT 64
> +
> +#define TX_URB_NUM 10
> +#define TX_SS_URB_NUMTX_URB_NUM
> +#define TX_HS_URB_NUMTX_URB_NUM
> +#define TX_FS_URB_NUMTX_URB_NUM
> +
> +/* A single URB buffer must be large enough to hold a complete jumbo packet
> + */
> +#define TX_SS_URB_SIZE (32 * 1024)
wow, 32k per allocation! Only 30 of them I guess.
> +#define TX_HS_URB_SIZE (16 * 1024)
> +#define TX_FS_URB_SIZE (10 * 1024)
> +
> +#define RX_SS_URB_NUM30
> +#define RX_HS_URB_NUM10
> +#define RX_FS_URB_NUM10
> +#define RX_SS_URB_SIZE TX_SS_URB_SIZE
> +#define RX_HS_URB_SIZE TX_HS_URB_SIZE
> +#define RX_FS_URB_SIZE TX_FS_URB_SIZE
> +
> +#define SS_BURST_CAP_SIZERX_SS_URB_SIZE
> +#define SS_BULK_IN_DELAY 0x2000
> +#define HS_BURST_CAP_SIZERX_HS_URB_SIZE
> +#define HS_BULK_IN_DELAY 0x2000
> +#define FS_BURST_CAP_SIZERX_FS_URB_SIZE
> +#define FS_BULK_IN_DELAY 0x2000
> +
> +#define TX_CMD_LEN 8
> +#define TX_SKB_MIN_LEN (TX_CMD_LEN + ETH_HLEN)
> +#define LAN78XX_TSO_SIZE(dev)((dev)->tx_urb_size -
> TX_SKB_MIN_LEN)
> +
> +#define RX_CMD_LEN 10
> +#define RX_SKB_MIN_LEN (RX_CMD_LEN + ETH_HLEN)
> +#define RX_MAX_FRAME_LEN(mtu)((mtu) + ETH_HLEN + VLAN_HLEN)
> +
> +#define LAN78XX_MIN_MTU MIN_IPV4_DGRAM
> +#define LAN78XX_MAX_MTU MAX_SINGLE_PACKET_SIZE
> +
> /* USB related defines */
> #define BULK_IN_PIPE 1
> #define BULK_OUT_PIPE2
> @@ -366,15 +404,22 @@ struct lan78xx_net {
> struct usb_interface*intf;
> void*driver_priv;
>
> - int rx_qlen;
> - int tx_qlen;
> + int tx_pend_data_len;
> + int n_tx_urbs;
> + int n_rx_urbs;
> + int rx_urb_size;
> + int tx_urb_size;
> +
> + struct sk_buff_head rxq_free;
> + struct sk_buff_head r
Re: [net-next v3 00/14] Add Marvell CN10K support
Geetha sowjanya wrote: > v2-v3 > Reposting as a single thread. FYI, it didn't work, suggest you try adding the git-send-email option (via git-config) sendemail.thread=true sendemail.chainreplyto=false And you can test locally by using first using git send-email to export to mbox and checking for References and In-Reply-to headers. Then sending for real. Good luck!
Re: [PATCH RESEND v3 net-next 4/5] net: use the new dev_page_is_reusable() instead of private versions
Alexander Lobakin wrote:
> Now we can remove a bunch of identical functions from the drivers and
> make them use common dev_page_is_reusable(). All {,un}likely() checks
> are omitted since it's already present in this helper.
> Also update some comments near the call sites.
>
> Suggested-by: David Rientjes
> Suggested-by: Jakub Kicinski
> Cc: John Hubbard
> Signed-off-by: Alexander Lobakin
I don't know why it was missed in the series update, but:
Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next v5 1/2] net: mhi-net: Add re-aggregation of fragmented packets
Loic Poulain wrote: > When device side MTU is larger than host side MTU, the packets > (typically rmnet packets) are split over multiple MHI transfers. > In that case, fragments must be re-aggregated to recover the packet > before forwarding to upper layer. > > A fragmented packet result in -EOVERFLOW MHI transaction status for > each of its fragments, except the final one. Such transfer was > previously considered as error and fragments were simply dropped. > > This change adds re-aggregation mechanism using skb chaining, via > skb frag_list. > > A warning (once) is printed since this behavior usually comes from > a misconfiguration of the device (e.g. modem MTU). > > Signed-off-by: Loic Poulain > --- > v2: use zero-copy skb chaining instead of skb_copy_expand. > v3: Fix nit in commit msg + remove misleading inline comment for frag_list > v4: no change > v5: reword/fix commit subject > Acked-by: Jesse Brandeburg
Re: [PATCH net-next] net: Return the correct errno code
Zheng Yongjun wrote: > When kzalloc failed, should return ENOMEM rather than ENOBUFS. All these patches have the same subject and description, couldn't they just be part of a single series with a good cover letter? I'm not saying make them a single patch, because that is bad for bisection, but having them as a single series means we review related changes at one time, and can comment on them as a group.
Re: [PATCH] octeontx2-af: remove unneeded semicolon
Yang Li wrote: > Eliminate the following coccicheck warning: > ./drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c:272:2-3: > Unneeded semicolon > ./drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:1809:3-4: > Unneeded semicolon > ./drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:1788:3-4: > Unneeded semicolon > ./drivers/net/ethernet/marvell/octeontx2/af/rvu.c:1326:2-3: Unneeded > semicolon > > Reported-by: Abaci Robot > Signed-off-by: Yang Li > --- > drivers/net/ethernet/marvell/octeontx2/af/rvu.c | 2 +- > drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c | 4 ++-- > drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) Trivial patch, recommend net-next as it's not a critical fix, Yang, please include the targeted tree when sending like [PATCH net-next] otherwise, for net-next: Reviewed-by: Jesse Brandeburg
Re: [PATCH net] hv_netvsc: Reset the RSC count if NVSP_STAT_FAIL in netvsc_receive()
Andrea Parri (Microsoft) wrote:
> Commit 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V
> values") added validation to rndis_filter_receive_data() (and
> rndis_filter_receive()) which introduced NVSP_STAT_FAIL-scenarios where
> the count is not updated/reset. Fix this omission, and prevent similar
> scenarios from occurring in the future.
>
> Reported-by: Juan Vazquez
> Signed-off-by: Andrea Parri (Microsoft)
> Cc: "David S. Miller"
> Cc: Jakub Kicinski
> Cc: netdev@vger.kernel.org
> Fixes: 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V
> values")
Reviewed-by: Jesse Brandeburg
Re: [PATCH][next] net: hns3: remove redundant null check of an array
Colin King wrote:
> From: Colin Ian King
>
> The null check of filp->f_path.dentry->d_iname is redundant because
> it is an array of DNAME_INLINE_LEN chars and cannot be a null. Fix
> this by removing the null check.
>
> Addresses-Coverity: ("Array compared against 0")
> Fixes: 04987ca1b9b6 ("net: hns3: add debugfs support for tm nodes, priority
> and qset info")
> Signed-off-by: Colin Ian King
Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next v4 1/2] net: mhi-net: Add de-aggeration support
Loic Poulain wrote: > When device side MTU is larger than host side MTU, the packets > (typically rmnet packets) are split over multiple MHI transfers. > In that case, fragments must be re-aggregated to recover the packet > before forwarding to upper layer. > > A fragmented packet result in -EOVERFLOW MHI transaction status for > each of its fragments, except the final one. Such transfer was > previously considered as error and fragments were simply dropped. > > This change adds re-aggregation mechanism using skb chaining, via > skb frag_list. > > A warning (once) is printed since this behavior usually comes from > a misconfiguration of the device (e.g. modem MTU). > > Signed-off-by: Loic Poulain > --- > v2: use zero-copy skb chaining instead of skb_copy_expand. > v3: Fix nit in commit msg + remove misleading inline comment for frag_list > v4: no change apologies for the nit, can you please fix the spelling of aggregation in the subject?
Re: [PATCH][next] net/mlx5e: Fix spelling mistake "Unknouwn" -> "Unknown"
Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in a netdev_warn message. Fix it. > > Signed-off-by: Colin Ian King Trivial patch, looks fine! Reviewed-by: Jesse Brandeburg
Re: [PATCH 0/2] chelsio: cxgb: Use threaded interrupts for deferred work
Sebastian Andrzej Siewior wrote: > Patch #2 fixes an issue in which del_timer_sync() and tasklet_kill() is > invoked from the interrupt handler. This is probably a rare error case > since it disables interrupts / the card in that case. > Patch #1 converts a worker to use a threaded interrupt which is then > also used in patch #2 instead adding another worker for this task (and > flush_work() to synchronise vs rmmod). > > This has been only compile tested. Hi! Thanks for your patch. Do all drivers that use worker threads need to convert like this or only some? In future revisions, please indicate the tree you're targeting, net or net-next. ie [PATCH net-next v1] I'd also invert the two paragraphs and talk about patch #1 first. Jesse
Re: [Patch v3 net-next 7/7] octeontx2-pf: ethtool physical link configuration
Hariprasad Kelam wrote: > From: Christina Jacob > > Register set_link_ksetting callback with driver such that > link configurations parameters like advertised mode,speed, duplex > and autoneg can be configured. > > below command > ethtool -s eth0 advertise 0x1 speed 10 duplex full autoneg on > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 6/7] octeontx2-pf: ethtool physical link status
Hariprasad Kelam wrote: > From: Christina Jacob > > Register get_link_ksettings callback to get link status information > from the driver. As virtual function (vf) shares same physical link > same API is used for both the drivers and for loop back drivers > simply returns the fixed values as its does not have physical link. > > ethtool eth3 > Settings for eth3: > Supported ports: [ ] > Supported link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Half 1000baseT/Full > 1baseKR/Full > 1000baseX/Full > Supports auto-negotiation: No > Supported FEC modes: BaseR RS > Advertised link modes: Not reported > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: None > > ethtool lbk0 > Settings for lbk0: > Speed: 10Mb/s > Duplex: Full > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam besides the slightly long lines, looks good. Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 5/7] octeontx2-af: advertised link modes support on cgx
Hariprasad Kelam wrote: > From: Christina Jacob > > CGX supports setting advertised link modes on physical link. > This patch adds support to derive cgx mode from ethtool > link mode and pass it to firmware to configure the same. > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 4/7] octeontx2-af: Physical link configuration support
Hariprasad Kelam wrote: > From: Christina Jacob > > CGX LMAC, the physical interface support link configuration parameters > like speed, auto negotiation, duplex etc. Firmware saves these into > memory region shared between firmware and this driver. > > This patch adds mailbox handler set_link_mode, fw_data_get to > configure and read these parameters. > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
Hariprasad Kelam wrote: > From: Felix Manlunas > > This patch adds support to fetch fec stats from PHY. The stats are > put in the shared data struct fwdata. A PHY driver indicates > that it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats > > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in sync > with firmware's enum list. > > Signed-off-by: Felix Manlunas > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Kovvuri Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support
Hariprasad Kelam wrote: > From: Christina Jacob > > Add ethtool support to configure fec modes baser/rs and > support to fecth FEC stats from CGX as well PHY. > > Configure fec mode > - ethtool --set-fec eth0 encoding rs/baser/off/auto > Query fec mode > - ethtool --show-fec eth0 > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 1/7] octeontx2-af: forward error correction configuration
Hariprasad Kelam wrote: > From: Christina Jacob > > CGX block supports forward error correction modes baseR > and RS. This patch adds support to set encoding mode > and to read corrected/uncorrected block counters > > Adds new mailbox handlers set_fec to configure encoding modes > and fec_stats to read counters and also increase mbox timeout > to accomdate firmware command response timeout. > > Along with new CGX_CMD_SET_FEC command add other commands to > sync with kernel enum list with firmware. > > Signed-off-by: Christina Jacob > Signed-off-by: Sunil Goutham > Signed-off-by: Hariprasad Kelam Reviewed-by: Jesse Brandeburg
Re: [Patch v3 net-next 0/7] ethtool support for fec and link configuration
Hariprasad Kelam wrote: > This series of patches add support for forward error correction(fec) and > physical link configuration. Patches 1&2 adds necessary mbox handlers for fec > mode configuration request and to fetch stats. Patch 3 registers driver > callbacks for fec mode configuration and display. Patch 4&5 adds support of > mbox > handlers for configuring link parameters like speed/duplex and autoneg etc. > Patche 6&7 registers driver callbacks for physical link configuration. For the series, in addition to the fact that Willem already replied to the previous posting of v3 that it looked good. Reviewed-by: Jesse Brandeburg
Re: [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage
Alexander Lobakin wrote: > page_is_pfmemalloc() is used mostly by networking drivers to test > if a page can be considered for reusing/recycling. > It doesn't write anything to the struct page itself, so its sole > argument can be constified, as well as the first argument of > skb_propagate_pfmemalloc(). > In Page Pool core code, it can be simply inlined instead. > Most of the callers from NIC drivers were just doppelgangers of > the same condition tests. Derive them into a new common function > do deduplicate the code. This is a useful cleanup! Thanks. For the series: Reviewed-by: Jesse Brandeburg
Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
Alexander Lobakin wrote: > A bunch of drivers test the page before reusing/recycling for two > common conditions: > - if a page was allocated under memory pressure (pfmemalloc page); > - if a page was allocated at a distant memory node (to exclude >slowdowns). > > Introduce and use a new common function for doing this and eliminate > all functions-duplicates from drivers. > > Suggested-by: David Rientjes > Signed-off-by: Alexander Lobakin > --- > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++ > drivers/net/ethernet/intel/fm10k/fm10k_main.c | 9 ++--- > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 15 +-- > drivers/net/ethernet/intel/iavf/iavf_txrx.c | 15 +-- > drivers/net/ethernet/intel/ice/ice_txrx.c | 11 +-- > drivers/net/ethernet/intel/igb/igb_main.c | 7 +-- > drivers/net/ethernet/intel/igc/igc_main.c | 7 +-- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 +-- > drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 +-- > drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +-- > include/linux/skbuff.h| 15 +++ > 11 files changed, 27 insertions(+), 83 deletions(-) For the patch, and esp. for the Intel drivers: Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next 1/2] ice: drop dead code in ice_receive_skb()
Eric Dumazet wrote: > From: Eric Dumazet > > napi_gro_receive() can never return GRO_DROP > > GRO_DROP can only be returned from napi_gro_frags() > which is the other NAPI GRO entry point. > > Followup patch will remove GRO_DROP, because drivers > are not supposed to call napi_gro_frags() if prior > napi_get_frags() has failed. > > Note that I have left the gro_dropped variable. I leave to ice > maintainers the decision to further remove it from ethtool -S results. > > Signed-off-by: Eric Dumazet > Cc: Jesse Brandeburg Acked-by: Jesse Brandeburg Jakub or David, you're welcome to apply directly as part of this series. The original code went into the kernel right as the code to remove GRO_DROP returns went in just before, but the reviews crossed each other and no-one (especially me :-( ) caught it. for reference: commit 0e00c05fa72554c86d7c7e0f538ec83bfe277c91 Merge: b18e9834f7b2 045790b7bc66 Author: David S. Miller Date: Thu Jun 25 16:16:21 2020 -0700 Subject: Merge branch 'napi_gro_receive-caller-return-value-cleanups'
Re: [PATCH net-next v1 1/2] net: core: count drops from GRO
Eric Dumazet wrote: > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -6071,6 +6071,7 @@ static gro_result_t napi_skb_finish(struct > > napi_struct *napi, > > break; > > > > case GRO_DROP: > > + atomic_long_inc(&skb->dev->rx_dropped); > > kfree_skb(skb); > > break; > > > > @@ -6159,6 +6160,7 @@ static gro_result_t napi_frags_finish(struct > > napi_struct *napi, > > break; > > > > case GRO_DROP: > > + atomic_long_inc(&skb->dev->rx_dropped); > > napi_reuse_skb(napi, skb); > > break; > > > > > This is not needed. I think we should clean up ice instead. My patch 2 already did that. I was trying to address the fact that I'm *actually seeing* GRO_DROP return codes coming back from stack. I'll try to reproduce that issue again that I saw. Maybe modern kernels don't have the problem as frequently or at all. > Drivers are supposed to have allocated the skb (using > napi_get_frags()) before calling napi_gro_frags() ice doesn't use napi_get_frags/napi_gro_frags, so I'm not sure how this is relevant. > Only napi_gro_frags() would return GRO_DROP, but we supposedly could > crash at that point, since a driver is clearly buggy. seems unlikely since we don't call those functions. > We probably can remove GRO_DROP completely, assuming lazy drivers are fixed. This might be ok, but doesn't explain why I was seeing this return code (which was the whole reason I was trying to count them), however I may have been running on a distro kernel from redhat/centos 8 when I was seeing these events. I haven't fully completed spelunking all the different sources, but might be able to follow down the rabbit hole further. > diff --git a/net/core/dev.c b/net/core/dev.c > index > 8fa739259041aaa03585b5a7b8ebce862f4b7d1d..c9460c9597f1de51957fdcfc7a64ca45bce5af7c > 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6223,9 +6223,6 @@ gro_result_t napi_gro_frags(struct napi_struct *napi) > gro_result_t ret; > struct sk_buff *skb = napi_frags_skb(napi); > > - if (!skb) > - return GRO_DROP; > - > trace_napi_gro_frags_entry(skb); > > ret = napi_frags_finish(napi, skb, dev_gro_receive(napi, skb)); This change (noted from your other patches is fine), and a likely improvement, thanks for sending those!
Re: [PATCH net-next v1 1/2] net: core: count drops from GRO
Shannon Nelson wrote: > On 1/6/21 1:55 PM, Jesse Brandeburg wrote: > > When drivers call the various receive upcalls to receive an skb > > to the stack, sometimes that stack can drop the packet. The good > > news is that the return code is given to all the drivers of > > NET_RX_DROP or GRO_DROP. The bad news is that no drivers except > > the one "ice" driver that I changed, check the stat and increment > > If the stack is dropping the packet, isn't it up to the stack to track > that, perhaps with something that shows up in netstat -s? We don't > really want to make the driver responsible for any drops that happen > above its head, do we? I totally agree! In patch 2/2 I revert the driver-specific changes I had made in an earlier patch, and this patch *was* my effort to make the stack show the drops. Maybe I wasn't clear. I'm seeing packets disappear during TCP workloads, and this GRO_DROP code was the source of the drops (I see it returning infrequently but regularly) The driver processes the packet but the stack never sees it, and there were no drop counters anywhere tracking it.
[PATCH net-next v1 2/2] ice: remove GRO drop accounting
The driver was counting GRO drops but now that the stack
does it with the previous patch, the driver can drop
all the logic. The driver keeps the dev_dbg message in order
to do optional detailed tracing.
This mostly undoes commit a8fffd7ae9a5 ("ice: add useful statistics").
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/ice/ice.h | 1 -
drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 -
drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
drivers/net/ethernet/intel/ice/ice_txrx.h | 1 -
drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 2 --
5 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h
b/drivers/net/ethernet/intel/ice/ice.h
index 56725356a17b..dde850045e7e 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -256,7 +256,6 @@ struct ice_vsi {
u32 tx_busy;
u32 rx_buf_failed;
u32 rx_page_failed;
- u32 rx_gro_dropped;
u16 num_q_vectors;
u16 base_vector;/* IRQ base for OS reserved vectors */
enum ice_vsi_type type;
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 9e8e9531cd87..025c0a13e724 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -59,7 +59,6 @@ static const struct ice_stats ice_gstrings_vsi_stats[] = {
ICE_VSI_STAT("rx_unknown_protocol", eth_stats.rx_unknown_protocol),
ICE_VSI_STAT("rx_alloc_fail", rx_buf_failed),
ICE_VSI_STAT("rx_pg_alloc_fail", rx_page_failed),
- ICE_VSI_STAT("rx_gro_dropped", rx_gro_dropped),
ICE_VSI_STAT("tx_errors", eth_stats.tx_errors),
ICE_VSI_STAT("tx_linearize", tx_linearize),
ICE_VSI_STAT("tx_busy", tx_busy),
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index c52b9bb0e3ab..e157a2b4fcb9 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5314,7 +5314,6 @@ static void ice_update_vsi_ring_stats(struct ice_vsi *vsi)
vsi->tx_linearize = 0;
vsi->rx_buf_failed = 0;
vsi->rx_page_failed = 0;
- vsi->rx_gro_dropped = 0;
rcu_read_lock();
@@ -5329,7 +5328,6 @@ static void ice_update_vsi_ring_stats(struct ice_vsi *vsi)
vsi_stats->rx_bytes += bytes;
vsi->rx_buf_failed += ring->rx_stats.alloc_buf_failed;
vsi->rx_page_failed += ring->rx_stats.alloc_page_failed;
- vsi->rx_gro_dropped += ring->rx_stats.gro_dropped;
}
/* update XDP Tx rings counters */
@@ -5361,7 +5359,7 @@ void ice_update_vsi_stats(struct ice_vsi *vsi)
ice_update_eth_stats(vsi);
cur_ns->tx_errors = cur_es->tx_errors;
- cur_ns->rx_dropped = cur_es->rx_discards + vsi->rx_gro_dropped;
+ cur_ns->rx_dropped = cur_es->rx_discards;
cur_ns->tx_dropped = cur_es->tx_discards;
cur_ns->multicast = cur_es->rx_multicast;
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h
b/drivers/net/ethernet/intel/ice/ice_txrx.h
index ff1a1cbd078e..6ce2046fc349 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -193,7 +193,6 @@ struct ice_rxq_stats {
u64 non_eop_descs;
u64 alloc_page_failed;
u64 alloc_buf_failed;
- u64 gro_dropped; /* GRO returned dropped */
};
/* this enum matches hardware bits and is meant to be used by DYN_CTLN
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
index bc2f4390b51d..3601b7d8abe5 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
@@ -192,8 +192,6 @@ ice_receive_skb(struct ice_ring *rx_ring, struct sk_buff
*skb, u16 vlan_tag)
(vlan_tag & VLAN_VID_MASK))
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tag);
if (napi_gro_receive(&rx_ring->q_vector->napi, skb) == GRO_DROP) {
- /* this is tracked separately to help us debug stack drops */
- rx_ring->rx_stats.gro_dropped++;
netdev_dbg(rx_ring->netdev, "Receive Queue %d: Dropped packet
from GRO\n",
rx_ring->q_index);
}
--
2.29.2
[PATCH net-next v1 0/2] GRO drop accounting
Add some accounting for when the stack drops a packet that a driver tried to indicate with a gro* call. This helps users track where packets might have disappeared to and will show up in the netdevice stats that already exist. After that, remove the driver specific workaround that was added to do the same, just scoped too small. Jesse Brandeburg (2): net: core: count drops from GRO ice: remove GRO drop accounting drivers/net/ethernet/intel/ice/ice.h | 1 - drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 - drivers/net/ethernet/intel/ice/ice_main.c | 4 +--- drivers/net/ethernet/intel/ice/ice_txrx.h | 1 - drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 2 -- net/core/dev.c| 2 ++ 6 files changed, 3 insertions(+), 8 deletions(-) -- 2.29.2
[PATCH net-next v1 1/2] net: core: count drops from GRO
When drivers call the various receive upcalls to receive an skb to the stack, sometimes that stack can drop the packet. The good news is that the return code is given to all the drivers of NET_RX_DROP or GRO_DROP. The bad news is that no drivers except the one "ice" driver that I changed, check the stat and increment the dropped count. This is currently leading to packets that arrive at the edge interface and are fully handled by the driver and then mysteriously disappear. Rather than fix all drivers to increment the drop stat when handling the return code, emulate the already existing statistic update for NET_RX_DROP events for the two GRO_DROP locations, and increment the dev->rx_dropped associated with the skb. Signed-off-by: Jesse Brandeburg Cc: Eric Dumazet Cc: Jamal Hadi Salim --- net/core/dev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/core/dev.c b/net/core/dev.c index 8fa739259041..ef34043a9550 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6071,6 +6071,7 @@ static gro_result_t napi_skb_finish(struct napi_struct *napi, break; case GRO_DROP: + atomic_long_inc(&skb->dev->rx_dropped); kfree_skb(skb); break; @@ -6159,6 +6160,7 @@ static gro_result_t napi_frags_finish(struct napi_struct *napi, break; case GRO_DROP: + atomic_long_inc(&skb->dev->rx_dropped); napi_reuse_skb(napi, skb); break; -- 2.29.2
Re: [PATCH 1/1] ice: fix array overflow on receiving too many fragments for a packet
Xiaohui Zhang wrote:
> From: Zhang Xiaohui
>
> If the hardware receives an oversized packet with too many rx fragments,
> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
> This becomes especially visible if it corrupts the freelist pointer of
> a slab page.
As I replied to the ionic patch, please justify this with how you found
it and how you reproduced a problem. Resend the patches as a series so
we can discuss them as one change.
>
> Signed-off-by: Zhang Xiaohui
> ---
> drivers/net/ethernet/intel/ice/ice_txrx.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c
> b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index eae75260f..f0f034fa5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -823,8 +823,12 @@ ice_add_rx_frag(struct ice_ring *rx_ring, struct
> ice_rx_buf *rx_buf,
>
> if (!size)
> return;
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buf->page,
> + struct skb_shared_info *shinfo = skb_shinfo(skb);
> +
> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
> + skb_add_rx_frag(skb, shinfo, rx_buf->page,
> rx_buf->page_offset, size, truesize);
> + }
The driver is using 2kB receive buffers, and can chain them together up
to a max receive size of 9126 bytes (or so), so how can we receive more
than 18 fragments? Please explain your logic
>
> /* page is being used so we must update the page offset */
> ice_rx_buf_adjust_pg_offset(rx_buf, truesize);
Your patch doesn't compile. You must compile test and explain your
patches better.
CC [M] drivers/net/ethernet/intel/ice//ice_main.o
CC [M] drivers/net/ethernet/intel/ice//ice_controlq.o
CC [M] drivers/net/ethernet/intel/ice//ice_common.o
CC [M] drivers/net/ethernet/intel/ice//ice_nvm.o
CC [M] drivers/net/ethernet/intel/ice//ice_switch.o
CC [M] drivers/net/ethernet/intel/ice//ice_sched.o
CC [M] drivers/net/ethernet/intel/ice//ice_base.o
CC [M] drivers/net/ethernet/intel/ice//ice_lib.o
CC [M] drivers/net/ethernet/intel/ice//ice_txrx_lib.o
CC [M] drivers/net/ethernet/intel/ice//ice_txrx.o
drivers/net/ethernet/intel/ice//ice_txrx.c: In function ‘ice_add_rx_frag’:
drivers/net/ethernet/intel/ice//ice_txrx.c:829:2: warning: ISO C90 forbids
mixed declarations and code [-Wdeclaration-after-statement]
829 | struct skb_shared_info *shinfo = skb_shinfo(skb);
| ^~
drivers/net/ethernet/intel/ice//ice_txrx.c:832:24: warning: passing argument 2
of ‘skb_add_rx_frag’ makes integer from pointer without a cast
[-Wint-conversion]
832 | skb_add_rx_frag(skb, shinfo, rx_buf->page,
|^~
||
|struct skb_shared_info *
In file included from ./include/linux/if_ether.h:19,
from ./include/uapi/linux/ethtool.h:19,
from ./include/linux/ethtool.h:18,
from ./include/linux/netdevice.h:37,
from ./include/trace/events/xdp.h:8,
from ./include/linux/bpf_trace.h:5,
from drivers/net/ethernet/intel/ice//ice_txrx.c:8:
./include/linux/skbuff.h:2182:47: note: expected ‘int’ but argument is of type
‘struct skb_shared_info *’
2182 | void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int
off,
| ^
Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet
Xiaohui Zhang wrote:
> From: Zhang Xiaohui
>
> If the hardware receives an oversized packet with too many rx fragments,
> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
> This becomes especially visible if it corrupts the freelist pointer of
> a slab page.
>
> Signed-off-by: Zhang Xiaohui
Hi, thanks for your patch.
It appears this is a part of a series of patches (at least this one and
one to the ice driver) - please send as one series, with a cover letter
explanation.
Please justify how this is a bug and how this is found / reproduced.
I'll respond separately to the ice driver patch as I don't know this
hardware and it's limits, but I suspect that you've tried to fix a bug
where there was none. (It seems like something a code scanner might find
and be confused about)
> ---
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 169ac4f54..a3e274c65 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue
> *q,
>
> dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr),
> PAGE_SIZE, DMA_FROM_DEVICE);
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + struct skb_shared_info *shinfo = skb_shinfo(skb);
you can't declare variables in the middle of a code flow in C, did you
compile this?
> +
> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
> + skb_add_rx_frag(skb, shinfo->nr_frags,
> page_info->page, 0, frag_len, PAGE_SIZE);
> + }
> page_info->page = NULL;
> page_info++;
> i--;
Re: [net-next v3 05/15] ice: create flow profile
Alexander Duyck wrote: > > > I'm not sure this logic is correct. Can the flow director rules > > > handle > > > a field that is removed? Last I knew it couldn't. If that is the case > > > you should be using ACL for any case in which a full mask is not > > > provided. So in your tests below you could probably drop the check > > > for > > > zero as I don't think that is a valid case in which flow director > > > would work. > > > > > > > I'm not sure what you meant by a field that is removed, but Flow > > Director can handle reduced input sets. Flow Director is able to handle > > 0 mask, full mask, and less than 4 tuples. ACL is needed/used only when > > a partial mask rule is requested. > > So historically speaking with flow director you are only allowed one > mask because it determines the inputs used to generate the hash that > identifies the flow. So you are only allowed one mask for all flows > because changing those inputs would break the hash mapping. > > Normally this ends up meaning that you have to do like what we did in > ixgbe and disable ATR and only allow one mask for all inputs. I > believe for i40e they required that you always use a full 4 tuple. I > didn't see something like that here. As such you may want to double > check that you can have a mix of flow director rules that are using 1 > tuple, 2 tuples, 3 tuples, and 4 tuples as last I knew you couldn't. > Basically if you had fields included they had to be included for all > the rules on the port or device depending on how the tables are set > up. The ice driver hardware is quite a bit more capable than the ixgbe or i40e hardware, and uses a limited set of ACL rules to support different sets of masks. We have some limits on the number of masks and the number of fields that we can simultaneously support, but I think that is pretty normal for limited hardware resources. Let's just say that if the code doesn't work on an E810 card then we messed up and we'll have to fix it. :-) Thanks for the review! Hope this helps...
Re: [PATCH V4 net-next 0/4] net: hns3: updates for -next
Huazhong Tan wrote: > There are several updates relating to the interrupt coalesce for > the HNS3 ethernet driver. > > #1 adds support for QL(quantity limiting, interrupt coalesce >based on the frame quantity). > #2 queries the maximum value of GL from the firmware instead of >a fixed value in code. > #3 adds support for 1us unit GL(gap limiting, interrupt coalesce >based on the gap time). > #4 renames gl_adapt_enable in struct hns3_enet_coalesce to fit >its new usage. > > change log: > V4 - remove #5~#10 from this series, which needs more discussion. > V3 - fix a typo error in #1 reported by Jakub Kicinski. > rewrite #9 commit log. > remove #11 from this series. > V2 - reorder #2 & #3 to fix compiler error. > fix some checkpatch warnings in #10 & #11. For the series: Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next resend 1/2] enetc: Fix endianness issues for enetc_ethtool
Claudiu Manoil wrote: > These particular fields are specified in the H/W reference > manual as having network byte order format, so enforce big > endian annotation for them and clear the related sparse > warnings in the process. > > Signed-off-by: Claudiu Manoil Thanks for fixing these warnings! Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next v2] net: don't include ethtool.h from netdevice.h
Jakub Kicinski wrote: > linux/netdevice.h is included in very many places, touching any > of its dependecies causes large incremental builds. > > Drop the linux/ethtool.h include, linux/netdevice.h just needs > a forward declaration of struct ethtool_ops. > > Fix all the places which made use of this implicit include. > > Acked-by: Johannes Berg > Signed-off-by: Jakub Kicinski Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next v2 1/2] ethtool: Add CMIS 4.0 module type to UAPI
Moshe Shemesh wrote: > From: Vladyslav Tarasiuk > > CMIS 4.0 document describes a universal EEPROM memory layout, which is > used for some modules such as DSFP, OSFP and QSFP-DD modules. In order > to distinguish them in userspace from existing standards, add > corresponding values. > > CMIS 4.0 EERPOM memory includes mandatory and optional pages, the max typo? s/EERPOM/EEPROM > read length 768B includes passive and active cables mandatory pages. > > Signed-off-by: Vladyslav Tarasiuk > Reviewed-by: Moshe Shemesh rest was ok.
Re: [PATCH net-next 0/3] mvneta: access skb_shared_info only on last frag
Lorenzo Bianconi wrote: > Build skb_shared_info on mvneta_rx_swbm stack and sync it to xdp_buff > skb_shared_info area only on the last fragment. > Avoid avoid unnecessary xdp_buff initialization in mvneta_rx_swbm routine. > This a preliminary series to complete xdp multi-buff in mvneta driver. > > Lorenzo Bianconi (3): > net: mvneta: avoid unnecessary xdp_buff initialization > net: mvneta: move skb_shared_info in mvneta_xdp_put_buff > net: mvneta: alloc skb_shared_info on the mvneta_rx_swbm stack > > drivers/net/ethernet/marvell/mvneta.c | 55 +-- > 1 file changed, 35 insertions(+), 20 deletions(-) > For the series: Reviewed-by: Jesse Brandeburg
Re: [PATCH net] iavf: fix error return code in iavf_init_get_resources()
Zhang Changzhong wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: b66c7bc1cd4d ("iavf: Refactor init state machine")
> Reported-by: Hulk Robot
> Signed-off-by: Zhang Changzhong
Thanks!
Reviewed-by: Jesse Brandeburg
Re: Subject: [PATCH net] drivers: net: ixgbe: Fix *_ipsec_offload_ok():, Use ip_hdr family
Christian Langrock wrote: Please fix your subject, remove the word 'Subject: ' > Xfrm_dev_offload_ok() is called with the unencrypted SKB. So in case of > interfamily ipsec traffic (IPv4-in-IPv6 and IPv6 in IPv4) the check > assumes the wrong family of the skb (IP family of the state). > With this patch the ip header of the SKB is used to determine the > family. > missing "Fixes: " line? It's useful here because I think this looks like a good candidate for stable bug fix. > Signed-off-by: Christian Langrock > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 2 +- > drivers/net/ethernet/intel/ixgbevf/ipsec.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) The patch looks ok otherwise, thanks!
Re: [PATCH net] ixgbe: fix probing of multi-port devices with one MDIO
Jakub Kicinski wrote: > On Sat, 17 Oct 2020 01:50:59 +0200 Ian Kumlien wrote: > > On Sat, Oct 17, 2020 at 1:20 AM Jakub Kicinski wrote: > > > Ian reports that after upgrade from v5.8.14 to v5.9 only one > > > of his 4 ixgbe netdevs appear in the system. > > > > > > Quoting the comment on ixgbe_x550em_a_has_mii(): > > > * Returns true if hw points to lowest numbered PCI B:D.F x550_em_a > > > device in > > > * the SoC. There are up to 4 MACs sharing a single MDIO bus on the > > > x550em_a, > > > * but we only want to register one MDIO bus. > > > > > > This matches the symptoms, since the return value from > > > ixgbe_mii_bus_init() is no longer ignored we need to handle > > > the higher ports of x550em without an error. > > > > Nice, that fixes it! > > > > You can add a: > > Tested-by: Ian Kumlien > > Will do, thanks! > > Tony, should I apply directly to net? Thank you Kuba! Seems like a pretty straight forward bug-fix. I recommend you apply it directly. Acked-by: Jesse Brandeburg
Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()
Vladimir Oltean wrote: > On Fri, Oct 16, 2020 at 02:36:25PM -0700, Jesse Brandeburg wrote: > > > Signed-off-by: zhudi > > > > Kernel documentation says for you to use your real name, please do so, > > unless you're a rock star and have officially changed your name to > > zhudi. I apologize for seeming off-putting, my goal was to add a little levity here, but I know that email does a bad job of transmitting my intent, and I will do better. > > Well, his real name is probably 朱棣, and the pinyin transliteration > system doesn't really insist on separating 朱 (zhu) from 棣 (di), or on > capitalizing any of twose words, so I'm not sure what your point is. > Would you prefer his sign-off to read 朱棣 ? Ah, thanks Vladimir for explaining the difference. If this is common parlance for commit messages from our Chinese developers, please forgive me, I'm trying to balance obvious correctness against typical usage. I'll adjust my expectations for single word names, thanks! Jesse
Re: [PATCH 1/2] tools/include: Update if_link.h and netlink.h
Jakub Kicinski wrote:
> On Fri, 16 Oct 2020 14:23:48 -0700 Jesse Brandeburg wrote:
> > > These are tested to be the latest as part of the tools/lib/bpf build.
> >
> > But you didn't mention why you're making these changes, and you're
> > removing a lot of comments without explaining why/where there might be
> > a replacement or why the comments are useless. I now see that you're
> > adding actual kdoc which is good, except for the part where
> > you don't put kdoc on all the structures.
>
> Note that he's just syncing the uAPI headers to tools/
>
> The source of the change is here:
>
> 78a3ea555713 ("net: remove comments on struct rtnl_link_stats")
> 0db0c34cfbc9 ("net: tighten the definition of interface statistics")
Thanks Kuba, I'm not trying to be a hard ass, but the commit message
didn't say why he's making the change, and if I bisect back to this
and see "sync" as the commit message, I think I'd be stuck chasing
"sync to what?"
I guess that his changelog could just say what you said?
Proposed:
Sync the uAPI headers so that userspace and the kernel match. These
changes match the updates to the files in the tools directory that were
already updated by commits:
78a3ea555713 ("net: remove comments on struct rtnl_link_stats")
0db0c34cfbc9 ("net: tighten the definition of interface statistics")
Re: [RFC] Exempt multicast address from five-second neighbor lifetime
Hi Jeff,
Jeff Dike wrote:
Your subject should indicate net or net-next as the tree, please see:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
> Commit 58956317c8de guarantees arp table entries a five-second lifetime. We
> have some apps which make heavy use of multicast, and these can cause the
> table to overflow by filling it with multicast addresses which can't be GC-ed
> until their five seconds are up.
> This patch allows multicast addresses to be thrown out before they've lived
> out their five seconds.
Not sure how many patches you've submitted, but your commit message
should be wrapped at 68 or 72 characters or so.
>
> Signed-off-by: Jeff Dike
your triple-dash and a diffstat should be right here, did you hand edit
this mail instead of using git format-patch to generate it?
>
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 81ee17594c32..22ced1381ede 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -204,6 +204,7 @@ struct neigh_table {
> int (*pconstructor)(struct pneigh_entry *);
> void(*pdestructor)(struct pneigh_entry *);
> void(*proxy_redo)(struct sk_buff *skb);
> + int (*is_multicast)(const void *pkey);
> bool(*allow_add)(const struct net_device *dev,
>struct netlink_ext_ack *extack);
> char*id;
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 8e39e28b0a8d..9500d28a43b0 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -235,6 +235,8 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>
> write_lock(&n->lock);
> if ((n->nud_state == NUD_FAILED) ||
> + (tbl->is_multicast &&
> + tbl->is_multicast(n->primary_key)) ||
> time_after(tref, n->updated))
> remove = true;
> write_unlock(&n->lock);
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 687971d83b4e..110d6d408edc 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -79,6 +79,7 @@
> #include
> #include
> #include
> +#define __UAPI_DEF_IN_CLASS 1
Why is this added in the middle of the includes?
> #include
> #include
> #include
> @@ -125,6 +126,7 @@ static int arp_constructor(struct neighbour *neigh);
> static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb);
> static void arp_error_report(struct neighbour *neigh, struct sk_buff *skb);
> static void parp_redo(struct sk_buff *skb);
> +static int arp_is_multicast(const void *pkey);
>
> static const struct neigh_ops arp_generic_ops = {
> .family = AF_INET,
> @@ -156,6 +158,7 @@ struct neigh_table arp_tbl = {
> .key_eq = arp_key_eq,
> .constructor= arp_constructor,
> .proxy_redo = parp_redo,
> + .is_multicast = arp_is_multicast,
> .id = "arp_cache",
> .parms = {
> .tbl= &arp_tbl,
> @@ -928,6 +931,10 @@ static void parp_redo(struct sk_buff *skb)
> arp_process(dev_net(skb->dev), NULL, skb);
> }
>
> +static int arp_is_multicast(const void *pkey)
> +{
> + return IN_MULTICAST(htonl(*((u32 *) pkey)));
> +}
Why not just move this function up and skip the declaration above?
>
> /*
> * Receive an arp request from the device layer.
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 27f29b957ee7..b42c9314cc4e 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -81,6 +81,7 @@ static void ndisc_error_report(struct neighbour *neigh,
> struct sk_buff *skb);
> static int pndisc_constructor(struct pneigh_entry *n);
> static void pndisc_destructor(struct pneigh_entry *n);
> static void pndisc_redo(struct sk_buff *skb);
> +static int ndisc_is_multicast(const void *pkey);
>
> static const struct neigh_ops ndisc_generic_ops = {
> .family = AF_INET6,
> @@ -115,6 +116,7 @@ struct neigh_table nd_tbl = {
> .pconstructor = pndisc_constructor,
> .pdestructor = pndisc_destructor,
> .proxy_redo = pndisc_redo,
> + .is_multicast = ndisc_is_multicast,
> .allow_add = ndisc_allow_add,
> .id = "ndisc_cache",
> .parms = {
> @@ -1706,6 +1708,11 @@ static void pndisc_redo(struct sk_buff *skb)
> kfree_skb(skb);
> }
>
> +static int ndisc_is_multicast(const void *pkey)
> +{
> + return (((struct in6_addr *) pkey)->in6_u.u6_addr8[0] & 0xf0) == 0xf0;
> +}
> +
Again, just move this up above the first usage?
Does the above work on big and little endian, just seems suspicious
even though you're using a byte offset? Also I suspect this will
trigger a warning with sparse or with W=2 about pointer alignment.
Re: [PATCH net] nexthop: Fix performance regression in nexthop deletion
Ido Schimmel wrote:
> From: Ido Schimmel
>
> While insertion of 16k nexthops all using the same netdev ('dummy10')
> takes less than a second, deletion takes about 130 seconds:
>
> # time -p ip -b nexthop.batch
> real 0.29
> user 0.01
> sys 0.15
>
> # time -p ip link set dev dummy10 down
> real 131.03
> user 0.06
> sys 0.52
snip...
> Since nexthops are always deleted under RTNL, synchronize_net() can be
> used instead. It will call synchronize_rcu_expedited() which only blocks
> for several microseconds as opposed to multiple milliseconds like
> synchronize_rcu().
>
> With this patch deletion of 16k nexthops takes less than a second:
>
> # time -p ip link set dev dummy10 down
> real 0.12
> user 0.00
> sys 0.04
That's a nice result! Well done! I can't really speak to whether or not
there is any horrible side effect of using synchronize_rcu_expedited(),
but FWIW:
Reviewed-by: Jesse Brandeburg
>
> Tested with fib_nexthops.sh which includes torture tests that prompted
> the initial change:
>
> # ./fib_nexthops.sh
> ...
> Tests passed: 134
> Tests failed: 0
>
> Fixes: 90f33bffa382 ("nexthops: don't modify published nexthop groups")
> Signed-off-by: Ido Schimmel
Nice fix, good commit message, thanks!
> ---
> net/ipv4/nexthop.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 8c0f17c6863c..0dc43ad28eb9 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -845,7 +845,7 @@ static void remove_nexthop_from_groups(struct net *net,
> struct nexthop *nh,
> remove_nh_grp_entry(net, nhge, nlinfo);
>
> /* make sure all see the newly published array before releasing rtnl */
> - synchronize_rcu();
> + synchronize_net();
> }
>
> static void remove_nexthop_group(struct nexthop *nh, struct nl_info *nlinfo)
Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()
zhudi wrote:
> "ip addr show" command execute error when we have a physical
> network card with number of VFs larger than 247.
Oh man, this bug has been hurting us forever and I've tried to fix it
several times without much luck, so thanks for working on it!
CC: David Ahern
As he's mentioned this bug.
> The return value of if_nlmsg_size() in rtnl_calcit() will exceed
> range of u16 data type when any network cards has a larger number of
> VFs. rtnl_vfinfo_size() will significant increase needed dump size when
> the value of num_vfs is larger.
>
> Eventually we get a wrong value of min_ifinfo_dump_size because of overflow
> which decides the memory size needed by netlink dump and netlink_dump()
> will return -EMSGSIZE because of not enough memory was allocated.
>
> So fix it by promoting min_dump_alloc data type to u32 to
> avoid data overflow and it's also align with the data type of
> struct netlink_callback{}.min_dump_alloc which is assigned by
> return value of rtnl_calcit()
I defer to others here on whether this is an acceptable API change.
> Signed-off-by: zhudi
Kernel documentation says for you to use your real name, please do so,
unless you're a rock star and have officially changed your name to
zhudi.
> ---
> include/linux/netlink.h | 2 +-
> net/core/rtnetlink.c| 8
> 2 files changed, 5 insertions(+), 5 deletions(-)
Does it work without any changes to iproute2?
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index e3e49f0e5c13..0a7db41b9e42 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -230,7 +230,7 @@ struct netlink_dump_control {
> int (*done)(struct netlink_callback *);
> void *data;
> struct module *module;
> - u16 min_dump_alloc;
> + u32 min_dump_alloc;
> };
As long as nothing in the API depends on the length of this struct, it
should work.
Re: [PATCH net-next v2 0/9] net: ethernet: ti: am65-cpsw: add multi port support in mac-only mode
Grygorii Strashko wrote: > Hi > > This series adds multi-port support in mac-only mode (multi MAC mode) to TI > AM65x CPSW driver in preparation for enabling support for multi-port devices, > like Main CPSW0 on K3 J721E SoC or future CPSW3g on K3 AM64x SoC. For the series Reviewed-by: Jesse Brandeburg
Re: [PATCH 1/2] tools/include: Update if_link.h and netlink.h
Hi Ian,
Ian Rogers wrote:
> These are tested to be the latest as part of the tools/lib/bpf build.
But you didn't mention why you're making these changes, and you're
removing a lot of comments without explaining why/where there might be
a replacement or why the comments are useless. I now see that you're
adding actual kdoc which is good, except for the part where
you don't put kdoc on all the structures.
>
> Signed-off-by: Ian Rogers
> ---
> tools/include/uapi/linux/if_link.h | 269 +
> tools/include/uapi/linux/netlink.h | 107
> 2 files changed, 342 insertions(+), 34 deletions(-)
>
> diff --git a/tools/include/uapi/linux/if_link.h
> b/tools/include/uapi/linux/if_link.h
> index 781e482dc499..c4b23f06f69e 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -7,24 +7,23 @@
>
> /* This struct should be in sync with struct rtnl_link_stats64 */
Maybe you should put a "definitions are available in the comment above
rtnl_link_stats64" comment here?
> struct rtnl_link_stats {
> - __u32 rx_packets; /* total packets received */
> - __u32 tx_packets; /* total packets transmitted*/
> - __u32 rx_bytes; /* total bytes received */
> - __u32 tx_bytes; /* total bytes transmitted */
> - __u32 rx_errors; /* bad packets received */
> - __u32 tx_errors; /* packet transmit problems */
> - __u32 rx_dropped; /* no space in linux buffers*/
> - __u32 tx_dropped; /* no space available in linux */
> - __u32 multicast; /* multicast packets received */
> + __u32 rx_packets;
> + __u32 tx_packets;
> + __u32 rx_bytes;
Why is removing all the comments useful? You didn't make any useful
change here.
> + __u32 tx_bytes;
> + __u32 rx_errors;
> + __u32 tx_errors;
> + __u32 rx_dropped;
> + __u32 tx_dropped;
> + __u32 multicast;
> __u32 collisions;
> -
> /* detailed rx_errors: */
> __u32 rx_length_errors;
> - __u32 rx_over_errors; /* receiver ring buff overflow */
> - __u32 rx_crc_errors; /* recved pkt with crc error*/
> - __u32 rx_frame_errors;/* recv'd frame alignment error */
> - __u32 rx_fifo_errors; /* recv'r fifo overrun */
> - __u32 rx_missed_errors; /* receiver missed packet */
> + __u32 rx_over_errors;
> + __u32 rx_crc_errors;
Same comment as above.
> + __u32 rx_frame_errors;
> + __u32 rx_fifo_errors;
> + __u32 rx_missed_errors;
>
> /* detailed tx_errors */
> __u32 tx_aborted_errors;
> @@ -37,29 +36,200 @@ struct rtnl_link_stats {
> __u32 rx_compressed;
> __u32 tx_compressed;
>
> - __u32 rx_nohandler; /* dropped, no handler found*/
> + __u32 rx_nohandler;
And here too!
> };
>
> -/* The main device statistics structure */
> +/**
> + * struct rtnl_link_stats64 - The main device statistics structure.
> + *
> + * @rx_packets: Number of good packets received by the interface.
> + * For hardware interfaces counts all good packets received from the device
> + * by the host, including packets which host had to drop at various stages
> + * of processing (even in the driver).
Oh, I see now. A good commit message would have prevented me burping on
this so much.
> + *
> + * @tx_packets: Number of packets successfully transmitted.
> + * For hardware interfaces counts packets which host was able to
> successfully
> + * hand over to the device, which does not necessarily mean that packets
> + * had been successfully transmitted out of the device, only that device
> + * acknowledged it copied them out of host memory.
> + *
> + * @rx_bytes: Number of good received bytes, corresponding to @rx_packets.
> + *
> + * For IEEE 802.3 devices should count the length of Ethernet Frames
> + * excluding the FCS.
> + *
> + * @tx_bytes: Number of good transmitted bytes, corresponding to @tx_packets.
> + *
> + * For IEEE 802.3 devices should count the length of Ethernet Frames
> + * excluding the FCS.
> + *
> + * @rx_errors: Total number of bad packets received on this network device.
> + * This counter must include events counted by @rx_length_errors,
> + * @rx_crc_errors, @rx_frame_errors and other errors not otherwise
> + * counted.
> + *
> + * @tx_errors: Total number of transmit problems.
> + * This counter must include events counter by @tx_aborted_errors,
> + * @tx_carrier_errors, @tx_fifo_errors, @tx_heartbeat_errors,
> + * @tx_window_errors and other errors not otherwise counted.
> + *
> + * @rx_dropped: Number of packets received but not processed,
> + * e.g. due to lack of resources or unsupported protocol.
> + * For hardware interfaces this counter sh
Re: [net-next PATCH 01/10] octeontx2-af: Update get/set resource count functions
sundeep.l...@gmail.com wrote: > From: Subbaraya Sundeep > > Since multiple blocks of same type are present in > 98xx, modify functions which get resource count and > which update resource count to work with individual > block address instead of block type. > > Signed-off-by: Subbaraya Sundeep > Signed-off-by: Sunil Goutham > Signed-off-by: Rakesh Babu LGTM Reviewed-by: Jesse Brandeburg
[PATCH net-next v3 8/9] sfc: fix kdoc warning
From: Jesse Brandeburg
kernel-doc script as used by W=1, is confused by the macro
usage inside the header describing the efx_ptp_data struct.
drivers/net/ethernet/sfc/ptp.c:345: warning: Function parameter or member
'MC_CMD_PTP_IN_TRANSMIT_LENMAX' not described in 'efx_ptp_data'
After some discussion on the list, break this patch out to
a separate one, and fix the issue through a creative
macro declaration.
Signed-off-by: Jesse Brandeburg
Acked-by: Edward Cree
---
v3: unchanged, but it should be noted that the kernel-doc script
should be fixed by someone smarter than me, to not complain
about the original code.
v2: split this patch out and used custom fix to work around
kernel-doc script.
---
drivers/net/ethernet/sfc/mcdi.h | 1 +
drivers/net/ethernet/sfc/ptp.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index ef6d21e4bd0b..69c2924a147c 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -190,6 +190,7 @@ void efx_mcdi_sensor_event(struct efx_nic *efx, efx_qword_t
*ev);
* 32-bit-aligned. Also, on Siena we must copy to the MC shared
* memory strictly 32 bits at a time, so add any necessary padding.
*/
+#define MCDI_TX_BUF_LEN(_len) DIV_ROUND_UP((_len), 4)
#define _MCDI_DECLARE_BUF(_name, _len) \
efx_dword_t _name[DIV_ROUND_UP(_len, 4)]
#define MCDI_DECLARE_BUF(_name, _len) \
diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 2e8c4569f03b..aae208fe6b6e 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -326,7 +326,7 @@ struct efx_ptp_data {
struct work_struct pps_work;
struct workqueue_struct *pps_workwq;
bool nic_ts_enabled;
- _MCDI_DECLARE_BUF(txbuf, MC_CMD_PTP_IN_TRANSMIT_LENMAX);
+ efx_dword_t txbuf[MCDI_TX_BUF_LEN(MC_CMD_PTP_IN_TRANSMIT_LENMAX)];
unsigned int good_syncs;
unsigned int fast_syncs;
--
2.25.4
[PATCH net-next v3 4/9] drivers/net/ethernet: rid ethernet of no-prototype warnings
From: Jesse Brandeburg The W=1 builds showed a few files exporting functions (non-static) that were not prototyped. What actually happened is that there were prototypes, but the include file was forgotten in the implementation file. Add the include file and remove the warnings. Fixed Warnings: drivers/net/ethernet/cavium/liquidio/cn68xx_device.c:124:5: warning: no previous prototype for ‘lio_setup_cn68xx_octeon_device’ [-Wmissing-prototypes] drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:159:1: warning: no previous prototype for ‘octeon_pci_read_core_mem’ [-Wmissing-prototypes] drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:168:1: warning: no previous prototype for ‘octeon_pci_write_core_mem’ [-Wmissing-prototypes] drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:176:5: warning: no previous prototype for ‘octeon_read_device_mem64’ [-Wmissing-prototypes] drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:185:5: warning: no previous prototype for ‘octeon_read_device_mem32’ [-Wmissing-prototypes] drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:194:6: warning: no previous prototype for ‘octeon_write_device_mem32’ [-Wmissing-prototypes] drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c:453:6: warning: no previous prototype for ‘hclge_dcb_ops_set’ [-Wmissing-prototypes] Signed-off-by: Jesse Brandeburg --- v3: add warnings detail v2: first non-rfc version Full list of warnings: drivers/net/ethernet/cavium/liquidio/cn68xx_device.c:124:5: warning: no previous prototype for ‘lio_setup_cn68xx_octeon_device’ [-Wmissing-prototypes] 124 | int lio_setup_cn68xx_octeon_device(struct octeon_device *oct) | ^~ drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:159:1: warning: no previous prototype for ‘octeon_pci_read_core_mem’ [-Wmissing-prototypes] 159 | octeon_pci_read_core_mem(struct octeon_device *oct, | ^~~~ drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:168:1: warning: no previous prototype for ‘octeon_pci_write_core_mem’ [-Wmissing-prototypes] 168 | octeon_pci_write_core_mem(struct octeon_device *oct, | ^ drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:176:5: warning: no previous prototype for ‘octeon_read_device_mem64’ [-Wmissing-prototypes] 176 | u64 octeon_read_device_mem64(struct octeon_device *oct, u64 coreaddr) | ^~~~ drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:185:5: warning: no previous prototype for ‘octeon_read_device_mem32’ [-Wmissing-prototypes] 185 | u32 octeon_read_device_mem32(struct octeon_device *oct, u64 coreaddr) | ^~~~ drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c:194:6: warning: no previous prototype for ‘octeon_write_device_mem32’ [-Wmissing-prototypes] 194 | void octeon_write_device_mem32(struct octeon_device *oct, u64 coreaddr, | ^ drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c:453:6: warning: no previous prototype for ‘hclge_dcb_ops_set’ [-Wmissing-prototypes] 453 | void hclge_dcb_ops_set(struct hclge_dev *hdev) | ^ --- drivers/net/ethernet/cavium/liquidio/cn68xx_device.c | 1 + drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 1 + 2 files changed, 2 insertions(+) diff --git a/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c b/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c index cd5d5d6e7e5e..2a6d1cadac9e 100644 --- a/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c +++ b/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c @@ -25,6 +25,7 @@ #include "octeon_main.h" #include "cn66xx_regs.h" #include "cn66xx_device.h" +#include "cn68xx_device.h" #include "cn68xx_regs.h" #include "cn68xx_device.h" diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c index f990f6915226..3606240025a8 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c @@ -4,6 +4,7 @@ #include "hclge_main.h" #include "hclge_dcb.h" #include "hclge_tm.h" +#include "hclge_dcb.h" #include "hnae3.h" #define BW_PERCENT 100 -- 2.25.4
[PATCH net-next v3 6/9] drivers/net/ethernet: add some basic kdoc tags
From: Jesse Brandeburg A couple of drivers had a "generic documentation" section that would trigger a "can't understand" message from W=1 compiles. Fix by using correct DOC: tags in the generic sections. Fixed Warnings: drivers/net/ethernet/arc/emac_arc.c:4: info: Scanning doc for c drivers/net/ethernet/cadence/macb_pci.c:3: warning: missing initial short description on line: * Cadence GEM PCI wrapper. drivers/net/ethernet/cadence/macb_pci.c:3: info: Scanning doc for Cadence Signed-off-by: Jesse Brandeburg --- v3: add some warning detail v2: first non-rfc version --- drivers/net/ethernet/arc/emac_arc.c | 2 +- drivers/net/ethernet/cadence/macb_pci.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/arc/emac_arc.c b/drivers/net/ethernet/arc/emac_arc.c index 1c7736b7eaf7..800620b8f10d 100644 --- a/drivers/net/ethernet/arc/emac_arc.c +++ b/drivers/net/ethernet/arc/emac_arc.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later /** - * emac_arc.c - ARC EMAC specific glue layer + * DOC: emac_arc.c - ARC EMAC specific glue layer * * Copyright (C) 2014 Romain Perier * diff --git a/drivers/net/ethernet/cadence/macb_pci.c b/drivers/net/ethernet/cadence/macb_pci.c index cd7d0332cba3..35316c91f523 100644 --- a/drivers/net/ethernet/cadence/macb_pci.c +++ b/drivers/net/ethernet/cadence/macb_pci.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /** - * Cadence GEM PCI wrapper. + * DOC: Cadence GEM PCI wrapper. * * Copyright (C) 2016 Cadence Design Systems - https://www.cadence.com * -- 2.25.4
[PATCH net-next v3 3/9] drivers/net/ethernet: clean up unused assignments
From: Jesse Brandeburg As part of the W=1 compliation series, these lines all created warnings about unused variables that were assigned a value. Most of them are from register reads, but some are just picking up a return value from a function and never doing anything with it. Fixed warnings: .../ethernet/brocade/bna/bnad.c:3280:6: warning: variable ‘rx_count’ set but not used [-Wunused-but-set-variable] .../ethernet/brocade/bna/bnad.c:3280:6: warning: variable ‘rx_count’ set but not used [-Wunused-but-set-variable] .../ethernet/cortina/gemini.c:512:6: warning: variable ‘val’ set but not used [-Wunused-but-set-variable] .../ethernet/cortina/gemini.c:2110:21: warning: variable ‘config0’ set but not used [-Wunused-but-set-variable] .../ethernet/cavium/liquidio/octeon_device.c:1327:6: warning: variable ‘val32’ set but not used [-Wunused-but-set-variable] .../ethernet/cavium/liquidio/octeon_device.c:1358:6: warning: variable ‘val32’ set but not used [-Wunused-but-set-variable] .../ethernet/dec/tulip/media.c:322:8: warning: variable ‘setup’ set but not used [-Wunused-but-set-variable] .../ethernet/dec/tulip/de4x5.c:4928:13: warning: variable ‘r3’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:1652:7: warning: variable ‘dummy’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:1652:7: warning: variable ‘dummy’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:1652:7: warning: variable ‘dummy’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:1652:7: warning: variable ‘dummy’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:4981:6: warning: variable ‘rx_status’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:6510:6: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable] .../ethernet/micrel/ksz884x.c:6087: warning: cannot understand function prototype: 'struct hw_regs ' .../ethernet/microchip/lan743x_main.c:161:6: warning: variable ‘int_en’ set but not used [-Wunused-but-set-variable] .../ethernet/microchip/lan743x_main.c:1702:6: warning: variable ‘int_sts’ set but not used [-Wunused-but-set-variable] .../ethernet/microchip/lan743x_main.c:3041:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] .../ethernet/natsemi/ns83820.c:603:6: warning: variable ‘tbisr’ set but not used [-Wunused-but-set-variable] .../ethernet/natsemi/ns83820.c:1207:11: warning: variable ‘tanar’ set but not used [-Wunused-but-set-variable] .../ethernet/marvell/mvneta.c:754:6: warning: variable ‘dummy’ set but not used [-Wunused-but-set-variable] .../ethernet/neterion/vxge/vxge-traffic.c:33:6: warning: variable ‘val64’ set but not used [-Wunused-but-set-variable] .../ethernet/neterion/vxge/vxge-traffic.c:160:6: warning: variable ‘val64’ set but not used [-Wunused-but-set-variable] .../ethernet/neterion/vxge/vxge-traffic.c:490:6: warning: variable ‘val32’ set but not used [-Wunused-but-set-variable] .../ethernet/neterion/vxge/vxge-traffic.c:2378:6: warning: variable ‘val64’ set but not used [-Wunused-but-set-variable] .../ethernet/packetengines/yellowfin.c:1063:18: warning: variable ‘yf_size’ set but not used [-Wunused-but-set-variable] .../ethernet/realtek/8139cp.c:1242:6: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable] .../ethernet/mellanox/mlx4/en_tx.c:858:6: warning: variable ‘ring_cons’ set but not used [-Wunused-but-set-variable] .../ethernet/sis/sis900.c:792:6: warning: variable ‘status’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:878:11: warning: variable ‘rx_ev_pkt_type’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:877:23: warning: variable ‘rx_ev_mcast_pkt’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:877:7: warning: variable ‘rx_ev_hdr_type’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:876:7: warning: variable ‘rx_ev_other_err’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:1646:21: warning: variable ‘buftbl_min’ set but not used [-Wunused-but-set-variable] .../ethernet/sfc/falcon/farch.c:2535:32: warning: variable ‘spec’ set but not used [-Wunused-but-set-variable] .../ethernet/via/via-velocity.c:880:6: warning: variable ‘curr_status’ set but not used [-Wunused-but-set-variable] .../ethernet/ti/tlan.c:656:6: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable] .../ethernet/ti/davinci_emac.c:1230:6: warning: variable ‘num_tx_pkts’ set but not used [-Wunused-but-set-variable] .../ethernet/synopsys/dwc-xlgmac-common.c:516:8: warning: variable ‘str’ set but not used [-Wunused-but-set-variable] .../ethernet/ti/cpsw_new.c:1662:22: warning: variable ‘priv’ set but not used [-Wunused-but-set-variable] The register reads should be OK, because the current implementation of readl and friends will always execute ev
[PATCH net-next v3 0/9] make drivers/net/ethernet W=1 clean
From: Jesse Brandeburg The Goal: move to W=1 being default for drivers/net/ethernet, and then use automation to catch more code issues (warnings) being introduced. The status: Getting much closer but not quite done for all architectures. After applying the patches below, the drivers/net/ethernet directory can be built as modules with W=1 with no warnings (so far on x64_64 arch only!). As Jakub pointed out, there is much more work to do to clean up C=1, but that will be another series of changes. This series removes 1,247 warnings and hopefully allows the ethernet directory to move forward from here without more warnings being added. There is only one objtool warning now. This version drops one of the Intel patches, as I couldn't reproduce the original issue to document the warning. Some of these patches are already sent and tested on Intel Wired Lan, but the rest of the series titled drivers/net/ethernet affects other drivers. The changes are all pretty straightforward. Signed-off-by: Jesse Brandeburg Reviewed-by: Jakub Kicinski Reviewed-by: Saeed Mahameed --- DaveM: if you want to apply all the intel driver changes directly please go ahead. As part of testing this series I realized that I have ~1,500 more kdoc warnings to fix due to being in other arch or not compiled with my x86_64 .config. Feel free to run $ git ls-files *.[ch] | grep drivers/net/ethernet | xargs scripts/kernel-doc -none to see the remaining issues. changes in v3: - addressed comments, updated commit messages with warnings noted, updated 9/9 with a few extras that snuck in while I was working, dropped a patch that I couldn't identify the original warning for. changes in v2: - non-rfc - addressed list comments from Edward Cree, Jacob Keller and Vinicius Costa Gomes - re-split the Intel patches into functional and kdoc only - split out the sfc changes that generated discussion to a single patch. Jesse Brandeburg (9): intel-ethernet: clean up W=1 warnings in kdoc intel: handle unused assignments drivers/net/ethernet: clean up unused assignments drivers/net/ethernet: rid ethernet of no-prototype warnings drivers/net/ethernet: handle one warning explicitly drivers/net/ethernet: add some basic kdoc tags drivers/net/ethernet: remove incorrectly formatted doc sfc: fix kdoc warning drivers/net/ethernet: clean up mis-targeted comments drivers/net/ethernet/amazon/ena/ena_com.c | 2 +- .../aquantia/atlantic/hw_atl/hw_atl_b0.c | 2 +- drivers/net/ethernet/arc/emac_arc.c | 2 +- .../net/ethernet/atheros/atl1c/atl1c_main.c | 6 +- .../net/ethernet/atheros/atl1e/atl1e_main.c | 7 +- drivers/net/ethernet/atheros/atlx/atl1.c | 2 +- drivers/net/ethernet/atheros/atlx/atl2.c | 6 +- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 2 + .../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 6 +- .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 12 +- .../net/ethernet/broadcom/bnx2x/bnx2x_sp.c| 98 ++--- drivers/net/ethernet/brocade/bna/bfa_cee.c| 20 +- drivers/net/ethernet/brocade/bna/bfa_ioc.c| 8 +- drivers/net/ethernet/brocade/bna/bnad.c | 7 +- drivers/net/ethernet/cadence/macb_main.c | 6 +- drivers/net/ethernet/cadence/macb_pci.c | 2 +- drivers/net/ethernet/calxeda/xgmac.c | 2 + .../ethernet/cavium/liquidio/cn68xx_device.c | 1 + .../net/ethernet/cavium/liquidio/lio_core.c | 92 ++--- .../net/ethernet/cavium/liquidio/lio_main.c | 351 +- .../ethernet/cavium/liquidio/lio_vf_main.c| 158 .../ethernet/cavium/liquidio/octeon_console.c | 12 +- .../ethernet/cavium/liquidio/octeon_device.c | 4 +- .../ethernet/cavium/liquidio/octeon_droq.c| 2 +- .../ethernet/cavium/liquidio/octeon_mailbox.c | 5 +- .../net/ethernet/chelsio/cxgb3/cxgb3_main.c | 8 +- drivers/net/ethernet/chelsio/cxgb3/sge.c | 24 +- drivers/net/ethernet/chelsio/cxgb3/t3_hw.c| 5 +- drivers/net/ethernet/cisco/enic/enic_api.c| 2 +- .../net/ethernet/cisco/enic/enic_ethtool.c| 2 +- drivers/net/ethernet/cortina/gemini.c | 2 + drivers/net/ethernet/dec/tulip/de4x5.c| 4 +- drivers/net/ethernet/dec/tulip/media.c| 5 - drivers/net/ethernet/dnet.c | 2 +- drivers/net/ethernet/ethoc.c | 6 +- .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- drivers/net/ethernet/freescale/fec_ptp.c | 5 +- drivers/net/ethernet/freescale/fman/fman.c| 14 +- .../net/ethernet/freescale/fman/fman_muram.c | 6 +- .../net/ethernet/freescale/fman/fman_port.c | 23 +- drivers/net/ethernet/freescale/fman/mac.c | 4 +- drivers/net/ethernet/hisilicon/hns/hnae.c | 2 +- .../net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 34 +- .../ethernet/hisilicon/hns/hns_dsaf_main.c| 148 .../ethernet/his
[PATCH net-next v3 2/9] intel: handle unused assignments
From: Jesse Brandeburg Remove variables that were storing a return value from a register read or other read, where the return value wasn't used. Those conversions to remove the lvalue of the assignment should be safe because the readl memory mapped reads are marked volatile and should not be optimized out without an lvalue (I suspect a very long time ago this wasn't guaranteed as it is today). These changes are part of a separate patch to make it easier to review. Warnings Fixed: .../intel/e100.c:2596:9: warning: variable ‘err’ set but not used [-Wunused-but-set-variable] .../intel/ixgb/ixgb_hw.c:101:6: warning: variable ‘icr_reg’ set but not used [-Wunused-but-set-variable] .../intel/ixgb/ixgb_hw.c:277:6: warning: variable ‘ctrl_reg’ set but not used [-Wunused-but-set-variable] .../intel/ixgb/ixgb_hw.c:952:15: warning: variable ‘temp_reg’ set but not used [-Wunused-but-set-variable] .../intel/ixgb/ixgb_hw.c:1164:7: warning: variable ‘mdio_reg’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:132:6: warning: variable ‘ret_val’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:380:6: warning: variable ‘icr’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:2378:6: warning: variable ‘signal’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:2374:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:2373:6: warning: variable ‘rxcw’ set but not used [-Wunused-but-set-variable] .../intel/e1000/e1000_hw.c:4678:15: warning: variable ‘temp’ set but not used [-Wunused-but-set-variable] Signed-off-by: Jesse Brandeburg Tested-by: Aaron Brown --- v3: added summary of warnings fixed. Full list of warnings fixed /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e100.c: In function ‘e100_diag_test’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e100.c:2596:9: warning: variable ‘err’ set but not used [-Wunused-but-set-variable] 2596 | int i, err; | ^~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c: In function ‘e1000_reset_hw’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c:381:6: warning: variable ‘icr’ set but not used [-Wunused-but-set-variable] 381 | u32 icr; | ^~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c: In function ‘e1000_check_for_link’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c:2378:6: warning: variable ‘signal’ set but not used [-Wunused-but-set-variable] 2378 | u32 signal = 0; | ^~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c:2374:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] 2374 | u32 ctrl; | ^~~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c:2373:6: warning: variable ‘rxcw’ set but not used [-Wunused-but-set-variable] 2373 | u32 rxcw = 0; | ^~~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c: In function ‘e1000_clear_hw_cntrs’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/e1000/e1000_hw.c:4678:15: warning: variable ‘temp’ set but not used [-Wunused-but-set-variable] 4678 | volatile u32 temp; | ^~~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c: In function ‘ixgb_adapter_stop’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c:101:6: warning: variable ‘icr_reg’ set but not used [-Wunused-but-set-variable] 101 | u32 icr_reg; | ^~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c: In function ‘ixgb_init_hw’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c:277:6: warning: variable ‘ctrl_reg’ set but not used [-Wunused-but-set-variable] 277 | u32 ctrl_reg; | ^~~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c: In function ‘ixgb_clear_hw_cntrs’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c:952:15: warning: variable ‘temp_reg’ set but not used [-Wunused-but-set-variable] 952 | volatile u32 temp_reg; | ^~~~ /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c: In function ‘ixgb_optics_reset’: /home/jbrandeb/git/linux/drivers/net/ethernet/intel/ixgb/ixgb_hw.c:1164:7: warning: variable ‘mdio_reg’ set but not used [-Wunused-but-set-variable] 1164 | u16 mdio_reg; | ^~~~ --- drivers/net/ethernet/intel/e100.c | 6 +- drivers/net/ethernet/intel/e1000/e1000_hw.c | 139 +--- drivers/net/ethernet/intel/ixgb/ixgb_hw.c | 135 +-- 3 files changed, 131 insertions(+), 149 deletions(-) diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c index 79c7a92aed16..76bb77b4607a 100644 --- a/drivers/net/ethernet/intel/e100.c +++ b/drivers/net/ethernet/intel/e100.c @@ -2593
[PATCH net-next v3 1/9] intel-ethernet: clean up W=1 warnings in kdoc
From: Jesse Brandeburg This takes care of all of the trivial W=1 fixes in the Intel Ethernet drivers, which allows developers and maintainers to build more of the networking tree with more complete warning checks. There are three classes of kdoc warnings fixed: - cannot understand function prototype: 'x' - Excess function parameter 'x' description in 'y' - Function parameter or member 'x' not described in 'y' All of the changes were trivial comment updates on function headers. Inspired by Lee Jones' series of wireless work to do the same. Compile tested only, and passes simple test of $ git ls-files *.[ch] | egrep drivers/net/ethernet/intel | \ xargs scripts/kernel-doc -none Signed-off-by: Jesse Brandeburg Tested-by: Aaron Brown --- v3: added tested-by, united previous v1,v2 patches to intel drivers into this patch. Full list of warnings fixed: git ls-files *.[ch] | egrep drivers/net/ethernet/intel | xargs scripts/kernel-doc -none drivers/net/ethernet/intel/e100.c:391: warning: cannot understand function prototype: 'enum cb_command ' drivers/net/ethernet/intel/e1000/e1000_hw.c:1908: warning: Excess function parameter 'mii_reg' description in 'e1000_config_mac_to_phy' drivers/net/ethernet/intel/e1000/e1000_hw.c:2931: warning: Function parameter or member 'phy_data' not described in 'e1000_write_phy_reg' drivers/net/ethernet/intel/e1000/e1000_hw.c:2931: warning: Excess function parameter 'data' description in 'e1000_write_phy_reg' drivers/net/ethernet/intel/e1000/e1000_hw.c:4789: warning: Excess function parameter 'tx_packets' description in 'e1000_update_adaptive' drivers/net/ethernet/intel/e1000/e1000_hw.c:4789: warning: Excess function parameter 'total_collisions' description in 'e1000_update_adaptive' drivers/net/ethernet/intel/e1000/e1000_hw.c:5080: warning: Excess function parameter 'downshift' description in 'e1000_check_downshift' drivers/net/ethernet/intel/e1000/e1000_main.c:208: warning: Function parameter or member 'hw' not described in 'e1000_get_hw_dev' drivers/net/ethernet/intel/e1000/e1000_main.c:361: warning: Function parameter or member 'adapter' not described in 'e1000_configure' drivers/net/ethernet/intel/e1000/e1000_main.c:3495: warning: Function parameter or member 'txqueue' not described in 'e1000_tx_timeout' drivers/net/ethernet/intel/e1000/e1000_main.c:3794: warning: Function parameter or member 'napi' not described in 'e1000_clean' drivers/net/ethernet/intel/e1000/e1000_main.c:3794: warning: Function parameter or member 'budget' not described in 'e1000_clean' drivers/net/ethernet/intel/e1000/e1000_main.c:3794: warning: Excess function parameter 'adapter' description in 'e1000_clean' drivers/net/ethernet/intel/e1000/e1000_main.c:3825: warning: Function parameter or member 'tx_ring' not described in 'e1000_clean_tx_irq' drivers/net/ethernet/intel/e1000/e1000_main.c:3941: warning: Function parameter or member 'skb' not described in 'e1000_rx_checksum' drivers/net/ethernet/intel/e1000/e1000_main.c:3941: warning: Excess function parameter 'sk_buff' description in 'e1000_rx_checksum' drivers/net/ethernet/intel/e1000/e1000_main.c:3977: warning: Function parameter or member 'bi' not described in 'e1000_consume_page' drivers/net/ethernet/intel/e1000/e1000_main.c:3977: warning: Function parameter or member 'skb' not described in 'e1000_consume_page' drivers/net/ethernet/intel/e1000/e1000_main.c:3977: warning: Function parameter or member 'length' not described in 'e1000_consume_page' drivers/net/ethernet/intel/e1000/e1000_main.c:4015: warning: Function parameter or member 'stats' not described in 'e1000_tbi_adjust_stats' drivers/net/ethernet/intel/e1000/e1000_main.c:4556: warning: Function parameter or member 'rx_ring' not described in 'e1000_alloc_rx_buffers' drivers/net/ethernet/intel/e1000/e1000_main.c:4556: warning: Function parameter or member 'cleaned_count' not described in 'e1000_alloc_rx_buffers' drivers/net/ethernet/intel/e1000/e1000_main.c:4669: warning: Function parameter or member 'adapter' not described in 'e1000_smartspeed' drivers/net/ethernet/intel/e1000/e1000_main.c:4728: warning: Function parameter or member 'netdev' not described in 'e1000_ioctl' drivers/net/ethernet/intel/e1000/e1000_main.c:4728: warning: Function parameter or member 'ifr' not described in 'e1000_ioctl' drivers/net/ethernet/intel/e1000/e1000_main.c:4728: warning: Function parameter or member
[PATCH net-next v3 7/9] drivers/net/ethernet: remove incorrectly formatted doc
From: Jesse Brandeburg As part of the W=1 series for ethernet, these drivers were discovered to be using kdoc style comments but were not actually doing kdoc. The kernel uses kdoc style when documenting code, not doxygen or other styles. Fixed Warnings: drivers/net/ethernet/amazon/ena/ena_com.c:613: warning: Function parameter or member 'ena_dev' not described in 'ena_com_set_llq' drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c:1540: warning: Cannot understand * @brief Set VLAN filter table drivers/net/ethernet/xilinx/ll_temac_main.c:114: warning: Function parameter or member 'lp' not described in 'temac_indirect_busywait' drivers/net/ethernet/xilinx/ll_temac_main.c:129: warning: Function parameter or member 'lp' not described in 'temac_indirect_in32' drivers/net/ethernet/xilinx/ll_temac_main.c:129: warning: Function parameter or member 'reg' not described in 'temac_indirect_in32' drivers/net/ethernet/xilinx/ll_temac_main.c:147: warning: Function parameter or member 'lp' not described in 'temac_indirect_in32_locked' drivers/net/ethernet/xilinx/ll_temac_main.c:147: warning: Function parameter or member 'reg' not described in 'temac_indirect_in32_locked' drivers/net/ethernet/xilinx/ll_temac_main.c:172: warning: Function parameter or member 'lp' not described in 'temac_indirect_out32' drivers/net/ethernet/xilinx/ll_temac_main.c:172: warning: Function parameter or member 'reg' not described in 'temac_indirect_out32' drivers/net/ethernet/xilinx/ll_temac_main.c:172: warning: Function parameter or member 'value' not described in 'temac_indirect_out32' drivers/net/ethernet/xilinx/ll_temac_main.c:188: warning: Function parameter or member 'lp' not described in 'temac_indirect_out32_locked' drivers/net/ethernet/xilinx/ll_temac_main.c:188: warning: Function parameter or member 'reg' not described in 'temac_indirect_out32_locked' drivers/net/ethernet/xilinx/ll_temac_main.c:188: warning: Function parameter or member 'value' not described in 'temac_indirect_out32_locked' drivers/net/ethernet/xilinx/ll_temac_main.c:212: warning: Function parameter or member 'lp' not described in 'temac_dma_in32_be' drivers/net/ethernet/xilinx/ll_temac_main.c:212: warning: Function parameter or member 'reg' not described in 'temac_dma_in32_be' drivers/net/ethernet/xilinx/ll_temac_main.c:228: warning: Function parameter or member 'lp' not described in 'temac_dma_out32_be' drivers/net/ethernet/xilinx/ll_temac_main.c:228: warning: Function parameter or member 'reg' not described in 'temac_dma_out32_be' drivers/net/ethernet/xilinx/ll_temac_main.c:228: warning: Function parameter or member 'value' not described in 'temac_dma_out32_be' drivers/net/ethernet/xilinx/ll_temac_main.c:247: warning: Function parameter or member 'lp' not described in 'temac_dma_dcr_in' drivers/net/ethernet/xilinx/ll_temac_main.c:247: warning: Function parameter or member 'reg' not described in 'temac_dma_dcr_in' drivers/net/ethernet/xilinx/ll_temac_main.c:255: warning: Function parameter or member 'lp' not described in 'temac_dma_dcr_out' drivers/net/ethernet/xilinx/ll_temac_main.c:255: warning: Function parameter or member 'reg' not described in 'temac_dma_dcr_out' drivers/net/ethernet/xilinx/ll_temac_main.c:255: warning: Function parameter or member 'value' not described in 'temac_dma_dcr_out' drivers/net/ethernet/xilinx/ll_temac_main.c:265: warning: Function parameter or member 'lp' not described in 'temac_dcr_setup' drivers/net/ethernet/xilinx/ll_temac_main.c:265: warning: Function parameter or member 'op' not described in 'temac_dcr_setup' drivers/net/ethernet/xilinx/ll_temac_main.c:265: warning: Function parameter or member 'np' not described in 'temac_dcr_setup' drivers/net/ethernet/xilinx/ll_temac_main.c:300: warning: Function parameter or member 'ndev' not described in 'temac_dma_bd_release' drivers/net/ethernet/xilinx/ll_temac_main.c:330: warning: Function parameter or member 'ndev' not described in 'temac_dma_bd_init' drivers/net/ethernet/xilinx/ll_temac_main.c:600: warning: Function parameter or member 'ndev' not described in 'temac_setoptions' drivers/net/ethernet/xilinx/ll_temac_main.c:600: warning: Function parameter or member 'options' not described in 'temac_setoptions' Signed-off-by: Jesse Brandeburg --- v3: added warning detail v2: first non-rfc version --- drivers/net/ethernet/amazon/ena/ena_com.c | 2 +- .../aquantia/atla
[PATCH net-next v3 5/9] drivers/net/ethernet: handle one warning explicitly
From: Jesse Brandeburg While fixing the W=1 builds, this warning came up because the developers used a very tricky way to get structures initialized to a non-zero value, but this causes GCC to warn about an override. In this case the override was intentional, so just disable the warning for this code with a kernel macro that results in disabling the warning for compiles on GCC versions after 8. It is not appropriate to change the struct to initialize all the values as it will just add a lot more code for no value. The code is completely correct as is, we just want to acknowledge that this code could generate a warning and we're ok with that. NOTE: the __diag_ignore macro currently only accepts a second argument of 8 (version 8), it's either use this one or open code the pragma. Fixed Warnings example (all the same): drivers/net/ethernet/renesas/sh_eth.c:51:12: warning: initialized field overwritten [-Woverride-init] drivers/net/ethernet/renesas/sh_eth.c:52:12: warning: initialized field overwritten [-Woverride-init] drivers/net/ethernet/renesas/sh_eth.c:53:13: warning: initialized field overwritten [-Woverride-init] + 256 more... Signed-off-by: Jesse Brandeburg --- v3: change to __diag_* macros, add warning detail v2: first non-RFC version Full list of warnings: == drivers/net/ethernet/renesas/sh_eth.c:51:12: warning: initialized field overwritten [-Woverride-init] 51 | [EDSR] = 0x, |^~ drivers/net/ethernet/renesas/sh_eth.c:51:12: note: (near initialization for ‘sh_eth_offset_gigabit[0]’) drivers/net/ethernet/renesas/sh_eth.c:52:12: warning: initialized field overwritten [-Woverride-init] 52 | [EDMR] = 0x0400, |^~ drivers/net/ethernet/renesas/sh_eth.c:52:12: note: (near initialization for ‘sh_eth_offset_gigabit[1]’) drivers/net/ethernet/renesas/sh_eth.c:53:13: warning: initialized field overwritten [-Woverride-init] 53 | [EDTRR] = 0x0408, | ^~ drivers/net/ethernet/renesas/sh_eth.c:53:13: note: (near initialization for ‘sh_eth_offset_gigabit[2]’) drivers/net/ethernet/renesas/sh_eth.c:54:13: warning: initialized field overwritten [-Woverride-init] 54 | [EDRRR] = 0x0410, | ^~ drivers/net/ethernet/renesas/sh_eth.c:54:13: note: (near initialization for ‘sh_eth_offset_gigabit[3]’) drivers/net/ethernet/renesas/sh_eth.c:55:12: warning: initialized field overwritten [-Woverride-init] 55 | [EESR] = 0x0428, |^~ drivers/net/ethernet/renesas/sh_eth.c:55:12: note: (near initialization for ‘sh_eth_offset_gigabit[4]’) drivers/net/ethernet/renesas/sh_eth.c:56:13: warning: initialized field overwritten [-Woverride-init] 56 | [EESIPR] = 0x0430, | ^~ drivers/net/ethernet/renesas/sh_eth.c:56:13: note: (near initialization for ‘sh_eth_offset_gigabit[5]’) drivers/net/ethernet/renesas/sh_eth.c:57:13: warning: initialized field overwritten [-Woverride-init] 57 | [TDLAR] = 0x0010, | ^~ drivers/net/ethernet/renesas/sh_eth.c:57:13: note: (near initialization for ‘sh_eth_offset_gigabit[6]’) drivers/net/ethernet/renesas/sh_eth.c:58:13: warning: initialized field overwritten [-Woverride-init] 58 | [TDFAR] = 0x0014, | ^~ drivers/net/ethernet/renesas/sh_eth.c:58:13: note: (near initialization for ‘sh_eth_offset_gigabit[7]’) drivers/net/ethernet/renesas/sh_eth.c:59:13: warning: initialized field overwritten [-Woverride-init] 59 | [TDFXR] = 0x0018, | ^~ drivers/net/ethernet/renesas/sh_eth.c:59:13: note: (near initialization for ‘sh_eth_offset_gigabit[8]’) drivers/net/ethernet/renesas/sh_eth.c:60:13: warning: initialized field overwritten [-Woverride-init] 60 | [TDFFR] = 0x001c, | ^~ drivers/net/ethernet/renesas/sh_eth.c:60:13: note: (near initialization for ‘sh_eth_offset_gigabit[9]’) drivers/net/ethernet/renesas/sh_eth.c:61:13: warning: initialized field overwritten [-Woverride-init] 61 | [RDLAR] = 0x0030, | ^~ drivers/net/ethernet/renesas/sh_eth.c:61:13: note: (near initialization for ‘sh_eth_offset_gigabit[10]’) drivers/net/ethernet/renesas/sh_eth.c:62:13: warning: initialized field overwritten [-Woverride-init] 62 | [RDFAR] = 0x0034, | ^~ drivers/net/ethernet/renesas/sh_eth.c:62:13: note: (near initialization for ‘sh_eth_offset_gigabit[11]’) drivers/net/ethernet/renesas/sh_eth.c:63:13: warning: initialized field overwritten [-Woverride-init] 63 | [RDFXR] = 0x0038, | ^~ drivers/net/ethernet/renesas/sh_eth.c:63:13: note: (near initialization for ‘sh_eth_offset_gigabit[12]’) drivers/net/ethernet/renesas/sh_eth.c:64:13: warning: initialized field overwritten [-Woverride-init] 64 | [RDFFR] = 0x003c, | ^~ drivers/net/ethernet/renesas/sh_eth.c:64:13: note: (near initialization for ‘sh_eth_offset_
Re: [RFC][Patch v1 2/3] i40e: limit msix vectors based on housekeeping CPUs
Nitesh Narayan Lal wrote: > In a realtime environment, it is essential to isolate unwanted IRQs from > isolated CPUs to prevent latency overheads. Creating MSIX vectors only > based on the online CPUs could lead to a potential issue on an RT setup > that has several isolated CPUs but a very few housekeeping CPUs. This is > because in these kinds of setups an attempt to move the IRQs to the > limited housekeeping CPUs from isolated CPUs might fail due to the per > CPU vector limit. This could eventually result in latency spikes because > of the IRQ threads that we fail to move from isolated CPUs. > > This patch prevents i40e to add vectors only based on available > housekeeping CPUs by using num_housekeeping_cpus(). > > Signed-off-by: Nitesh Narayan Lal The driver changes are straightforward, but this isn't the only driver with this issue, right? I'm sure ixgbe and ice both have this problem too, you should fix them as well, at a minimum, and probably other vendors drivers: $ rg -c --stats num_online_cpus drivers/net/ethernet ... 50 files contained matches for this patch i40e Acked-by: Jesse Brandeburg
Re: [RFC][Patch v1 1/3] sched/isolation: API to get num of hosekeeping CPUs
Nitesh Narayan Lal wrote:
> Introduce a new API num_housekeeping_cpus(), that can be used to retrieve
> the number of housekeeping CPUs by reading an atomic variable
> __num_housekeeping_cpus. This variable is set from housekeeping_setup().
>
> This API is introduced for the purpose of drivers that were previously
> relying only on num_online_cpus() to determine the number of MSIX vectors
> to create. In an RT environment with large isolated but a fewer
> housekeeping CPUs this was leading to a situation where an attempt to
> move all of the vectors corresponding to isolated CPUs to housekeeping
> CPUs was failing due to per CPU vector limit.
>
> If there are no isolated CPUs specified then the API returns the number
> of all online CPUs.
>
> Signed-off-by: Nitesh Narayan Lal
> ---
> include/linux/sched/isolation.h | 7 +++
> kernel/sched/isolation.c| 23 +++
> 2 files changed, 30 insertions(+)
I'm not a scheduler expert, but a couple comments follow.
>
> diff --git a/include/linux/sched/isolation.h b/include/linux/sched/isolation.h
> index cc9f393e2a70..94c25d956d8a 100644
> --- a/include/linux/sched/isolation.h
> +++ b/include/linux/sched/isolation.h
> @@ -25,6 +25,7 @@ extern bool housekeeping_enabled(enum hk_flags flags);
> extern void housekeeping_affine(struct task_struct *t, enum hk_flags flags);
> extern bool housekeeping_test_cpu(int cpu, enum hk_flags flags);
> extern void __init housekeeping_init(void);
> +extern unsigned int num_housekeeping_cpus(void);
>
> #else
>
> @@ -46,6 +47,12 @@ static inline bool housekeeping_enabled(enum hk_flags
> flags)
> static inline void housekeeping_affine(struct task_struct *t,
> enum hk_flags flags) { }
> static inline void housekeeping_init(void) { }
> +
> +static unsigned int num_housekeeping_cpus(void)
> +{
> + return num_online_cpus();
> +}
> +
> #endif /* CONFIG_CPU_ISOLATION */
>
> static inline bool housekeeping_cpu(int cpu, enum hk_flags flags)
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 5a6ea03f9882..7024298390b7 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -13,6 +13,7 @@ DEFINE_STATIC_KEY_FALSE(housekeeping_overridden);
> EXPORT_SYMBOL_GPL(housekeeping_overridden);
> static cpumask_var_t housekeeping_mask;
> static unsigned int housekeeping_flags;
> +static atomic_t __num_housekeeping_cpus __read_mostly;
>
> bool housekeeping_enabled(enum hk_flags flags)
> {
> @@ -20,6 +21,27 @@ bool housekeeping_enabled(enum hk_flags flags)
> }
> EXPORT_SYMBOL_GPL(housekeeping_enabled);
>
> +/*
use correct kdoc style, and you get free documentation from your source
(you're so close!)
should be (note the first line and the function title line change to
remove parens:
/**
* num_housekeeping_cpus - Read the number of housekeeping CPUs.
*
* This function returns the number of available housekeeping CPUs
* based on __num_housekeeping_cpus which is of type atomic_t
* and is initialized at the time of the housekeeping setup.
*/
> + * num_housekeeping_cpus() - Read the number of housekeeping CPUs.
> + *
> + * This function returns the number of available housekeeping CPUs
> + * based on __num_housekeeping_cpus which is of type atomic_t
> + * and is initialized at the time of the housekeeping setup.
> + */
> +unsigned int num_housekeeping_cpus(void)
> +{
> + unsigned int cpus;
> +
> + if (static_branch_unlikely(&housekeeping_overridden)) {
> + cpus = atomic_read(&__num_housekeeping_cpus);
> + /* We should always have at least one housekeeping CPU */
> + BUG_ON(!cpus);
you need to crash the kernel because of this? maybe a WARN_ON? How did
the global even get set to the bad value? It's going to blame the poor
caller for this in the trace, but the caller likely had nothing to do
with setting the value incorrectly!
> + return cpus;
> + }
> + return num_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(num_housekeeping_cpus);
Re: [PATCH net] nfp: use correct define to return NONE fec
Jakub Kicinski wrote:
> struct ethtool_fecparam carries bitmasks not bit numbers.
> We want to return 1 (NONE), not 0.
>
> Fixes: 0d0870938337 ("nfp: implement ethtool FEC mode settings")
> Signed-off-by: Jakub Kicinski
Good catch!
Reviewed-by: Jesse Brandeburg
Re: [PATCH net-next v2 06/10] drivers/net/ethernet: handle one warning explicitly
Saeed Mahameed wrote:
> On Mon, 2020-09-14 at 18:44 -0700, Jesse Brandeburg wrote:
> > While fixing the W=1 builds, this warning came up because the
> > developers used a very tricky way to get structures initialized
> > to a non-zero value, but this causes GCC to warn about an
> > override. In this case the override was intentional, so just
> > disable the warning for this code with a macro that results
> > in disabling the warning for compiles on GCC versions after 8.
> >
> > NOTE: the __diag_ignore macro currently only accepts a second
> > argument of 8 (version 8)
> >
> > Signed-off-by: Jesse Brandeburg
> > ---
> > drivers/net/ethernet/renesas/sh_eth.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> > b/drivers/net/ethernet/renesas/sh_eth.c
> > index 586642c33d2b..c63304632935 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -45,6 +45,15 @@
> > #define SH_ETH_OFFSET_DEFAULTS \
> > [0 ... SH_ETH_MAX_REGISTER_OFFSET - 1] = SH_ETH_OFFSET_INVALID
> >
> > +/* use some intentionally tricky logic here to initialize the whole
> > struct to
> > + * 0x, but then override certain fields, requiring us to
> > indicate that we
> > + * "know" that there are overrides in this structure, and we'll need
> > to disable
> > + * that warning from W=1 builds. GCC has supported this option since
> > 4.2.X, but
> > + * the macros available to do this only define GCC 8.
> > + */
> > +__diag_push();
> > +__diag_ignore(GCC, 8, "-Woverride-init",
> > + "logic to initialize all and then override some is OK");
> > static const u16 sh_eth_offset_gigabit[SH_ETH_MAX_REGISTER_OFFSET] =
> > {
> > SH_ETH_OFFSET_DEFAULTS,
> >
> > @@ -332,6 +341,7 @@ static const u16
> > sh_eth_offset_fast_sh3_sh2[SH_ETH_MAX_REGISTER_OFFSET] = {
> >
> > [TSU_ADRH0] = 0x0100,
> > };
> > +__diag_pop();
> >
>
> I don't have any strong feeling against disabling compiler warnings,
> but maybe the right thing to do here is to initialize the gaps to the
> invalid value instead of pre-initializing the whole thing first and
> then setting up the valid values on the 2nd pass.
>
> I don't think there are too many gaps to fill, it is doable, so maybe
> add this as a comment to this driver maintainer so they could pickup
> the work from here.
added linux-renesas-soc list. @list, any comments on Saeed's comment
above?
Thanks,
Jesse
Re: [PATCH net-next v2 00/10] make drivers/net/ethernet W=1 clean
David Miller wrote: > Jesse, in all of these patches, I want to see the warning you are > fixing in the commit message. > > Especially for the sh_eth.c one because I have no idea what the > compiler is actually warning about just by reading your commit > message and patch on it's own. Ok, I'll respin with those added for the compiler warning fixes in particular, and some simplified descriptions of the classes of kdoc warnings.
Re: [PATCH net-next v2 00/10] make drivers/net/ethernet W=1 clean
Saeed Mahameed wrote: > Reviewed-by: Saeed Mahameed Thanks! In all fairness, Jakub reviewed this in v1 too but I made enough changes in v2 that I felt I had to drop the previous review ACKs. > Hi Jesse, > What was the criteria to select which drivers to enable in your .config > ? As Jakub said, I'm using allmodconfig on x86_64, but just yesterday discovered there was much more to fix because I ran the kernel-doc script directly on the source (those other things come from different ARCH= builds which limit allmodconfig) > I think we need some automation here and have a well known .config that > enables as many drivers as we can for static + compilation testing, > otherwise we are going to need to repeat this patch every 2-3 months. Totally agree! Jakub already has some cobbled together and is regularly running W=1 C=1 builds on all new patches. I found I could cross compile different ARCH targets to get (some of) the other warnings, or better yet just run the scripts/kernel-doc script directly in automation. > I know Jakub and Dave do some compilation testing before merging but i > don't know how much driver coverage they have and if they use a > specific .config or they just manually create one on demand.. > > bottom line, we need a bot after this series is applied. > All we need is to daily apply all ongoing patches to some testing > branch and let 0-DAY kernel test [1] run on it with whatever make > command we define and with all drivers enabled. Yes, that's the end goal and I think this moves us closer to that. A little more work remains before we go and turn all warnings on - as Andrew suggested in another reply. (it's also sometimes a losing game fighting against many compiler versions, etc). However, the zero-day bot reporting more results from W=1 compiles would *really* help (I looked at , but am having some troubles verifying that) > [1] https://lists.01.org/pipermail/kbuild-all > > > --- > > > > Q: Maybe I can fix the remaining warnings in a followup patch? If > > I try to put it on this series it will make it much larger > > (double).
[PATCH net-next v2 06/10] drivers/net/ethernet: handle one warning explicitly
While fixing the W=1 builds, this warning came up because the
developers used a very tricky way to get structures initialized
to a non-zero value, but this causes GCC to warn about an
override. In this case the override was intentional, so just
disable the warning for this code with a macro that results
in disabling the warning for compiles on GCC versions after 8.
NOTE: the __diag_ignore macro currently only accepts a second
argument of 8 (version 8)
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/renesas/sh_eth.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c
b/drivers/net/ethernet/renesas/sh_eth.c
index 586642c33d2b..c63304632935 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -45,6 +45,15 @@
#define SH_ETH_OFFSET_DEFAULTS \
[0 ... SH_ETH_MAX_REGISTER_OFFSET - 1] = SH_ETH_OFFSET_INVALID
+/* use some intentionally tricky logic here to initialize the whole struct to
+ * 0x, but then override certain fields, requiring us to indicate that we
+ * "know" that there are overrides in this structure, and we'll need to disable
+ * that warning from W=1 builds. GCC has supported this option since 4.2.X, but
+ * the macros available to do this only define GCC 8.
+ */
+__diag_push();
+__diag_ignore(GCC, 8, "-Woverride-init",
+ "logic to initialize all and then override some is OK");
static const u16 sh_eth_offset_gigabit[SH_ETH_MAX_REGISTER_OFFSET] = {
SH_ETH_OFFSET_DEFAULTS,
@@ -332,6 +341,7 @@ static const u16
sh_eth_offset_fast_sh3_sh2[SH_ETH_MAX_REGISTER_OFFSET] = {
[TSU_ADRH0] = 0x0100,
};
+__diag_pop();
static void sh_eth_rcv_snd_disable(struct net_device *ndev);
static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev);
--
2.28.0
[PATCH net-next v2 04/10] drivers/net/ethernet: clean up unused assignments
As part of the W=1 compliation series, these lines all created
warnings about unused variables that were assigned a value. Most
of them are from register reads, but some are just picking up
a return value from a function and never doing anything with it.
The register reads should be OK, because the current
implementation of readl and friends will always execute even
without an lvalue.
When it makes sense, just remove the lvalue assignment and the
local. Other times, just remove the offending code, and
occasionally, just mark the variable as maybe unused since it
could be used in an ifdef or debug scenario.
Only compile tested with W=1.
Signed-off-by: Jesse Brandeburg
---
v2: addressed some comments and got rid of a few kdoc changes that snuck
into this version.
---
drivers/net/ethernet/brocade/bna/bnad.c | 7 ++--
.../ethernet/cavium/liquidio/octeon_device.c | 9 +++---
drivers/net/ethernet/cortina/gemini.c | 6 ++--
drivers/net/ethernet/dec/tulip/de4x5.c| 4 +--
drivers/net/ethernet/dec/tulip/media.c| 5 ---
drivers/net/ethernet/dnet.c | 8 ++---
drivers/net/ethernet/freescale/fec_ptp.c | 3 +-
drivers/net/ethernet/marvell/mvneta.c | 7 ++--
drivers/net/ethernet/marvell/pxa168_eth.c | 3 +-
drivers/net/ethernet/mellanox/mlx4/en_tx.c| 2 +-
drivers/net/ethernet/micrel/ksz884x.c | 13 +++-
drivers/net/ethernet/microchip/lan743x_main.c | 9 ++
.../net/ethernet/neterion/vxge/vxge-traffic.c | 32 +++
.../ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c | 3 +-
drivers/net/ethernet/sfc/falcon/farch.c | 29 +++--
drivers/net/ethernet/sis/sis900.c | 5 ++-
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +--
.../net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
drivers/net/ethernet/ti/cpsw_new.c| 2 --
drivers/net/ethernet/ti/davinci_emac.c| 5 ++-
drivers/net/ethernet/ti/tlan.c| 4 +--
drivers/net/ethernet/via/via-velocity.c | 13
22 files changed, 59 insertions(+), 116 deletions(-)
diff --git a/drivers/net/ethernet/brocade/bna/bnad.c
b/drivers/net/ethernet/brocade/bna/bnad.c
index cc80bbbefe87..7e4e831d720f 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -3277,7 +3277,7 @@ bnad_change_mtu(struct net_device *netdev, int new_mtu)
{
int err, mtu;
struct bnad *bnad = netdev_priv(netdev);
- u32 rx_count = 0, frame, new_frame;
+ u32 frame, new_frame;
mutex_lock(&bnad->conf_mutex);
@@ -3293,12 +3293,9 @@ bnad_change_mtu(struct net_device *netdev, int new_mtu)
/* only when transition is over 4K */
if ((frame <= 4096 && new_frame > 4096) ||
(frame > 4096 && new_frame <= 4096))
- rx_count = bnad_reinit_rx(bnad);
+ bnad_reinit_rx(bnad);
}
- /* rx_count > 0 - new rx created
-* - Linux set err = 0 and return
-*/
err = bnad_mtu_set(bnad, new_frame);
if (err)
err = -EBUSY;
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
index ac32facaa427..fbde7c58c4db 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1324,7 +1324,7 @@ u64 lio_pci_readq(struct octeon_device *oct, u64 addr)
{
u64 val64;
unsigned long flags;
- u32 val32, addrhi;
+ u32 addrhi;
spin_lock_irqsave(&oct->pci_win_lock, flags);
@@ -1339,10 +1339,10 @@ u64 lio_pci_readq(struct octeon_device *oct, u64 addr)
writel(addrhi, oct->reg_list.pci_win_rd_addr_hi);
/* Read back to preserve ordering of writes */
- val32 = readl(oct->reg_list.pci_win_rd_addr_hi);
+ readl(oct->reg_list.pci_win_rd_addr_hi);
writel(addr & 0x, oct->reg_list.pci_win_rd_addr_lo);
- val32 = readl(oct->reg_list.pci_win_rd_addr_lo);
+ readl(oct->reg_list.pci_win_rd_addr_lo);
val64 = readq(oct->reg_list.pci_win_rd_data);
@@ -1355,7 +1355,6 @@ void lio_pci_writeq(struct octeon_device *oct,
u64 val,
u64 addr)
{
- u32 val32;
unsigned long flags;
spin_lock_irqsave(&oct->pci_win_lock, flags);
@@ -1365,7 +1364,7 @@ void lio_pci_writeq(struct octeon_device *oct,
/* The write happens when the LSB is written. So write MSB first. */
writel(val >> 32, oct->reg_list.pci_win_wr_data_hi);
/* Read the MSB to ensure ordering of writes. */
- val32 = readl(oct->reg_list.pci_win_wr_data_hi);
+ readl(oct->reg_list.pci_win_wr_data_hi);
writel(val & 0x, oct->reg_list.pci_win_wr_data_lo);
[PATCH net-next v2 02/10] intel-ethernet: clean up W=1 warnings in kdoc
This takes care of all of the trivial W=1 fixes in the Intel
Ethernet drivers, which allows developers and maintainers to
build more of the networking tree with more complete warning
checks.
Almost all of the changes were trivial comment updates on
function headers, with one debug message added to use a
variable after it's assignment.
Inspired by Lee Jones' series of wireless work to do the same.
Compile tested only.
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/e100.c | 2 +-
drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 +--
drivers/net/ethernet/intel/e1000/e1000_main.c | 39 +--
.../net/ethernet/intel/e1000e/80003es2lan.c | 1 -
drivers/net/ethernet/intel/e1000e/ich8lan.c | 16 +++---
drivers/net/ethernet/intel/e1000e/netdev.c| 50 ++-
drivers/net/ethernet/intel/e1000e/phy.c | 3 ++
drivers/net/ethernet/intel/e1000e/ptp.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_client.c | 2 -
drivers/net/ethernet/intel/i40e/i40e_common.c | 4 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 17 ---
drivers/net/ethernet/intel/i40e/i40e_ptp.c| 1 -
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 7 +--
.../ethernet/intel/i40e/i40e_virtchnl_pf.c| 9 ++--
drivers/net/ethernet/intel/iavf/iavf_main.c | 20
drivers/net/ethernet/intel/igb/e1000_82575.c | 6 +--
drivers/net/ethernet/intel/igb/e1000_i210.c | 5 +-
drivers/net/ethernet/intel/igb/e1000_mac.c| 1 +
drivers/net/ethernet/intel/igb/e1000_mbx.c| 1 +
drivers/net/ethernet/intel/igb/igb_main.c | 28 ++-
drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +--
drivers/net/ethernet/intel/igbvf/netdev.c | 17 +--
drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
drivers/net/ethernet/intel/igc/igc_ptp.c | 4 +-
drivers/net/ethernet/intel/ixgb/ixgb_main.c | 17 ---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 8 +--
.../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 +-
28 files changed, 171 insertions(+), 113 deletions(-)
diff --git a/drivers/net/ethernet/intel/e100.c
b/drivers/net/ethernet/intel/e100.c
index 36da059388dc..79c7a92aed16 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -384,7 +384,7 @@ enum cb_status {
cb_ok = 0x2000,
};
-/**
+/*
* cb_command - Command Block flags
* @cb_tx_nc: 0: controller does CRC (normal), 1: CRC from skb memory
*/
diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c
b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index 4e7a0810eaeb..fb8514078caf 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -139,6 +139,7 @@ static void e1000_phy_init_script(struct e1000_hw *hw)
* at the end of this routine.
*/
ret_val = e1000_read_phy_reg(hw, 0x2F5B, &phy_saved_data);
+ e_dbg("Reading PHY register 0x2F5B failed: %d\n", ret_val);
/* Disabled the PHY transmitter */
e1000_write_phy_reg(hw, 0x2F5B, 0x0003);
@@ -1897,7 +1898,6 @@ void e1000_config_collision_dist(struct e1000_hw *hw)
/**
* e1000_config_mac_to_phy - sync phy and mac settings
* @hw: Struct containing variables accessed by shared code
- * @mii_reg: data to write to the MII control register
*
* Sets MAC speed and duplex settings to reflect the those in the PHY
* The contents of the PHY register containing the needed information need to
@@ -2922,7 +2922,7 @@ static s32 e1000_read_phy_reg_ex(struct e1000_hw *hw, u32
reg_addr,
*
* @hw: Struct containing variables accessed by shared code
* @reg_addr: address of the PHY register to write
- * @data: data to write to the PHY
+ * @phy_data: data to write to the PHY
*
* Writes a value to a PHY register
*/
@@ -4778,8 +4778,6 @@ void e1000_reset_adaptive(struct e1000_hw *hw)
/**
* e1000_update_adaptive - update adaptive IFS
* @hw: Struct containing variables accessed by shared code
- * @tx_packets: Number of transmits since last callback
- * @total_collisions: Number of collisions since last callback
*
* Called during the callback/watchdog routine to update IFS value based on
* the ratio of transmits to collisions.
@@ -5064,8 +5062,6 @@ static s32 e1000_check_polarity(struct e1000_hw *hw,
/**
* e1000_check_downshift - Check if Downshift occurred
* @hw: Struct containing variables accessed by shared code
- * @downshift: output parameter : 0 - No Downshift occurred.
- *1 - Downshift occurred.
*
* returns: - E1000_ERR_XXX
*E1000_SUCCESS
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 1e6ec081fd9d..906591614553 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_
[PATCH net-next v2 03/10] intel: handle unused assignments
Remove variables that were storing a return value from a register
read or other read, where the return value wasn't used. Those
conversions to remove the lvalue of the assignment should be safe
because the readl memory mapped reads are marked volatile and
should not be optimized out without an lvalue (I suspect a very
long time ago this wasn't guaranteed as it is today).
These changes are part of a separate patch to make it easier to review.
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/e100.c | 6 +-
drivers/net/ethernet/intel/e1000/e1000_hw.c | 139 +---
drivers/net/ethernet/intel/ixgb/ixgb_hw.c | 135 +--
3 files changed, 131 insertions(+), 149 deletions(-)
diff --git a/drivers/net/ethernet/intel/e100.c
b/drivers/net/ethernet/intel/e100.c
index 79c7a92aed16..76bb77b4607a 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2593,7 +2593,7 @@ static void e100_diag_test(struct net_device *netdev,
{
struct ethtool_cmd cmd;
struct nic *nic = netdev_priv(netdev);
- int i, err;
+ int i;
memset(data, 0, E100_TEST_LEN * sizeof(u64));
data[0] = !mii_link_ok(&nic->mii);
@@ -2601,7 +2601,7 @@ static void e100_diag_test(struct net_device *netdev,
if (test->flags & ETH_TEST_FL_OFFLINE) {
/* save speed, duplex & autoneg settings */
- err = mii_ethtool_gset(&nic->mii, &cmd);
+ mii_ethtool_gset(&nic->mii, &cmd);
if (netif_running(netdev))
e100_down(nic);
@@ -2610,7 +2610,7 @@ static void e100_diag_test(struct net_device *netdev,
data[4] = e100_loopback_test(nic, lb_phy);
/* restore speed, duplex & autoneg settings */
- err = mii_ethtool_sset(&nic->mii, &cmd);
+ mii_ethtool_sset(&nic->mii, &cmd);
if (netif_running(netdev))
e100_up(nic);
diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c
b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index fb8514078caf..2120dacfd55c 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -378,7 +378,6 @@ s32 e1000_reset_hw(struct e1000_hw *hw)
{
u32 ctrl;
u32 ctrl_ext;
- u32 icr;
u32 manc;
u32 led_ctrl;
s32 ret_val;
@@ -503,7 +502,7 @@ s32 e1000_reset_hw(struct e1000_hw *hw)
ew32(IMC, 0x);
/* Clear any pending interrupt events. */
- icr = er32(ICR);
+ er32(ICR);
/* If MWI was previously enabled, reenable it. */
if (hw->mac_type == e1000_82542_rev2_0) {
@@ -2370,16 +2369,13 @@ static s32 e1000_check_for_serdes_link_generic(struct
e1000_hw *hw)
*/
s32 e1000_check_for_link(struct e1000_hw *hw)
{
- u32 rxcw = 0;
- u32 ctrl;
u32 status;
u32 rctl;
u32 icr;
- u32 signal = 0;
s32 ret_val;
u16 phy_data;
- ctrl = er32(CTRL);
+ er32(CTRL);
status = er32(STATUS);
/* On adapters with a MAC newer than 82544, SW Definable pin 1 will be
@@ -2388,12 +2384,9 @@ s32 e1000_check_for_link(struct e1000_hw *hw)
*/
if ((hw->media_type == e1000_media_type_fiber) ||
(hw->media_type == e1000_media_type_internal_serdes)) {
- rxcw = er32(RXCW);
+ er32(RXCW);
if (hw->media_type == e1000_media_type_fiber) {
- signal =
- (hw->mac_type >
-e1000_82544) ? E1000_CTRL_SWDPIN1 : 0;
if (status & E1000_STATUS_LU)
hw->get_link_status = false;
}
@@ -4675,78 +4668,76 @@ s32 e1000_led_off(struct e1000_hw *hw)
*/
static void e1000_clear_hw_cntrs(struct e1000_hw *hw)
{
- volatile u32 temp;
-
- temp = er32(CRCERRS);
- temp = er32(SYMERRS);
- temp = er32(MPC);
- temp = er32(SCC);
- temp = er32(ECOL);
- temp = er32(MCC);
- temp = er32(LATECOL);
- temp = er32(COLC);
- temp = er32(DC);
- temp = er32(SEC);
- temp = er32(RLEC);
- temp = er32(XONRXC);
- temp = er32(XONTXC);
- temp = er32(XOFFRXC);
- temp = er32(XOFFTXC);
- temp = er32(FCRUC);
-
- temp = er32(PRC64);
- temp = er32(PRC127);
- temp = er32(PRC255);
- temp = er32(PRC511);
- temp = er32(PRC1023);
- temp = er32(PRC1522);
-
- temp = er32(GPRC);
- temp = er32(BPRC);
- temp = er32(MPRC);
- temp = er32(GPTC);
- temp = er32(GORCL);
- temp = er32(GORCH);
- temp = er32(GOTCL);
- temp = er32(GOTCH);
- temp = er32(RNBC);
- temp = er32(RUC);
- temp = er32(RFC);
-
[PATCH net-next v2 08/10] drivers/net/ethernet: remove incorrectly formatted doc
As part of the W=1 series for ethernet, these drivers were discovered to be using kdoc style comments but were not actually doing kdoc. The kernel uses kdoc style when documenting code, not doxygen or other styles. Signed-off-by: Jesse Brandeburg --- drivers/net/ethernet/amazon/ena/ena_com.c | 2 +- .../aquantia/atlantic/hw_atl/hw_atl_b0.c | 2 +- drivers/net/ethernet/xilinx/ll_temac_main.c | 26 +-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c index 452e66b39a17..6967d40b5ac7 100644 --- a/drivers/net/ethernet/amazon/ena/ena_com.c +++ b/drivers/net/ethernet/amazon/ena/ena_com.c @@ -603,7 +603,7 @@ static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c return ret; } -/** +/* * Set the LLQ configurations of the firmware * * The driver provides only the enabled feature values to the device, diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c index 8941ac4df9e3..9f1b15077e7d 100644 --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c @@ -1536,7 +1536,7 @@ static int hw_atl_b0_hw_fl2_clear(struct aq_hw_s *self, return aq_hw_err_from_flags(self); } -/** +/* * @brief Set VLAN filter table * @details Configure VLAN filter table to accept (and assign the queue) traffic * for the particular vlan ids. diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c index 9a15f14daa47..60c199fcb91e 100644 --- a/drivers/net/ethernet/xilinx/ll_temac_main.c +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c @@ -106,7 +106,7 @@ static bool hard_acs_rdy_or_timeout(struct temac_local *lp, ktime_t timeout) */ #define HARD_ACS_RDY_POLL_NS (20 * NSEC_PER_MSEC) -/** +/* * temac_indirect_busywait - Wait for current indirect register access * to complete. */ @@ -121,7 +121,7 @@ int temac_indirect_busywait(struct temac_local *lp) return 0; } -/** +/* * temac_indirect_in32 - Indirect register read access. This function * must be called without lp->indirect_lock being held. */ @@ -136,7 +136,7 @@ u32 temac_indirect_in32(struct temac_local *lp, int reg) return val; } -/** +/* * temac_indirect_in32_locked - Indirect register read access. This * function must be called with lp->indirect_lock being held. Use * this together with spin_lock_irqsave/spin_lock_irqrestore to avoid @@ -164,7 +164,7 @@ u32 temac_indirect_in32_locked(struct temac_local *lp, int reg) return temac_ior(lp, XTE_LSW0_OFFSET); } -/** +/* * temac_indirect_out32 - Indirect register write access. This function * must be called without lp->indirect_lock being held. */ @@ -177,7 +177,7 @@ void temac_indirect_out32(struct temac_local *lp, int reg, u32 value) spin_unlock_irqrestore(lp->indirect_lock, flags); } -/** +/* * temac_indirect_out32_locked - Indirect register write access. This * function must be called with lp->indirect_lock being held. Use * this together with spin_lock_irqsave/spin_lock_irqrestore to avoid @@ -202,7 +202,7 @@ void temac_indirect_out32_locked(struct temac_local *lp, int reg, u32 value) WARN_ON(temac_indirect_busywait(lp)); } -/** +/* * temac_dma_in32_* - Memory mapped DMA read, these function expects a * register input that is based on DCR word addresses which are then * converted to memory mapped byte addresses. To be assigned to @@ -218,7 +218,7 @@ static u32 temac_dma_in32_le(struct temac_local *lp, int reg) return ioread32(lp->sdma_regs + (reg << 2)); } -/** +/* * temac_dma_out32_* - Memory mapped DMA read, these function expects * a register input that is based on DCR word addresses which are then * converted to memory mapped byte addresses. To be assigned to @@ -240,7 +240,7 @@ static void temac_dma_out32_le(struct temac_local *lp, int reg, u32 value) */ #ifdef CONFIG_PPC_DCR -/** +/* * temac_dma_dcr_in32 - DCR based DMA read */ static u32 temac_dma_dcr_in(struct temac_local *lp, int reg) @@ -248,7 +248,7 @@ static u32 temac_dma_dcr_in(struct temac_local *lp, int reg) return dcr_read(lp->sdma_dcrs, reg); } -/** +/* * temac_dma_dcr_out32 - DCR based DMA write */ static void temac_dma_dcr_out(struct temac_local *lp, int reg, u32 value) @@ -256,7 +256,7 @@ static void temac_dma_dcr_out(struct temac_local *lp, int reg, u32 value) dcr_write(lp->sdma_dcrs, reg, value); } -/** +/* * temac_dcr_setup - If the DMA is DCR based, then setup the address and * I/O functions */ @@ -293,7 +293,7 @@ static int temac_dcr_setup(struct temac_local *lp, struct platform_device *op, #endif -/** +/* * temac_dma_bd_release - Release buffer desc
[PATCH net-next v2 05/10] drivers/net/ethernet: rid ethernet of no-prototype warnings
The W=1 builds showed a few files exporting functions (non-static) that were not prototyped. What actually happened is that there were prototypes, but the include file was forgotten in the implementation file. Add the include file and remove the warnings. Signed-off-by: Jesse Brandeburg --- drivers/net/ethernet/cavium/liquidio/cn68xx_device.c | 1 + drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c | 1 + drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 1 + 3 files changed, 3 insertions(+) diff --git a/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c b/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c index 50b533ff58e6..30254e4cf70f 100644 --- a/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c +++ b/drivers/net/ethernet/cavium/liquidio/cn68xx_device.c @@ -25,6 +25,7 @@ #include "octeon_main.h" #include "cn66xx_regs.h" #include "cn66xx_device.h" +#include "cn68xx_device.h" #include "cn68xx_regs.h" static void lio_cn68xx_set_dpi_regs(struct octeon_device *oct) diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c b/drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c index 4c85ae643b7b..7ccab36143c1 100644 --- a/drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c +++ b/drivers/net/ethernet/cavium/liquidio/octeon_mem_ops.c @@ -22,6 +22,7 @@ #include "octeon_iq.h" #include "response_manager.h" #include "octeon_device.h" +#include "octeon_mem_ops.h" #define MEMOPS_IDX BAR1_INDEX_DYNAMIC_MAP diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c index d6c3952aba04..f28b8f3df857 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c @@ -3,6 +3,7 @@ #include "hclge_main.h" #include "hclge_tm.h" +#include "hclge_dcb.h" #include "hnae3.h" #define BW_PERCENT 100 -- 2.28.0
[PATCH net-next v2 07/10] drivers/net/ethernet: add some basic kdoc tags
A couple of drivers had a "generic documentation" section that would trigger a "can't understand" message from W=1 compiles. Fix by using correct DOC: tags. Signed-off-by: Jesse Brandeburg --- drivers/net/ethernet/arc/emac_arc.c | 2 +- drivers/net/ethernet/cadence/macb_pci.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/arc/emac_arc.c b/drivers/net/ethernet/arc/emac_arc.c index 1c7736b7eaf7..800620b8f10d 100644 --- a/drivers/net/ethernet/arc/emac_arc.c +++ b/drivers/net/ethernet/arc/emac_arc.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later /** - * emac_arc.c - ARC EMAC specific glue layer + * DOC: emac_arc.c - ARC EMAC specific glue layer * * Copyright (C) 2014 Romain Perier * diff --git a/drivers/net/ethernet/cadence/macb_pci.c b/drivers/net/ethernet/cadence/macb_pci.c index cd7d0332cba3..35316c91f523 100644 --- a/drivers/net/ethernet/cadence/macb_pci.c +++ b/drivers/net/ethernet/cadence/macb_pci.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /** - * Cadence GEM PCI wrapper. + * DOC: Cadence GEM PCI wrapper. * * Copyright (C) 2016 Cadence Design Systems - https://www.cadence.com * -- 2.28.0
[PATCH net-next v2 00/10] make drivers/net/ethernet W=1 clean
After applying the patches below, the drivers/net/ethernet directory can be built as modules with W=1 with no warnings (so far on x64_64 arch only!). As Jakub pointed out, there is much more work to do to clean up C=1, but that will be another series of changes. This series removes 1,283 warnings and hopefully allows the ethernet directory to move forward from here without more warnings being added. There is only one objtool warning now. Some of these patches are already sent to Intel Wired Lan, but the rest of the series titled drivers/net/ethernet affects other drivers. The changes are all pretty straightforward. As part of testing this series I realized that I have ~1,500 more kdoc warnings to fix due to being in other arch or not compiled with my x86_64 .config. Feel free to run $ 'git ls-files *.[ch] | grep drivers/net/ethernet | xargs scripts/kernel-doc -none' to see the remaining issues. --- Q: Maybe I can fix the remaining warnings in a followup patch? If I try to put it on this series it will make it much larger (double). changes in v2: - non-rfc - addressed list comments from Edward Cree, Jacob Keller and Vinicius Costa Gomes - re-split the Intel patches into functional and kdoc only - split out the sfc changes that generated discussion to a single patch. Jesse Brandeburg (10): i40e: prepare flash string in a simpler way intel-ethernet: clean up W=1 warnings in kdoc intel: handle unused assignments drivers/net/ethernet: clean up unused assignments drivers/net/ethernet: rid ethernet of no-prototype warnings drivers/net/ethernet: handle one warning explicitly drivers/net/ethernet: add some basic kdoc tags drivers/net/ethernet: remove incorrectly formatted doc sfc: fix kdoc warning drivers/net/ethernet: clean up mis-targeted comments drivers/net/ethernet/amazon/ena/ena_com.c | 2 +- .../aquantia/atlantic/hw_atl/hw_atl_b0.c | 2 +- drivers/net/ethernet/arc/emac_arc.c | 2 +- .../net/ethernet/atheros/atl1c/atl1c_main.c | 6 +- .../net/ethernet/atheros/atl1e/atl1e_main.c | 7 +- drivers/net/ethernet/atheros/atlx/atl1.c | 2 +- drivers/net/ethernet/atheros/atlx/atl2.c | 6 +- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 2 + .../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 6 +- .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 12 +- .../net/ethernet/broadcom/bnx2x/bnx2x_sp.c| 98 ++--- drivers/net/ethernet/brocade/bna/bfa_cee.c| 20 +- drivers/net/ethernet/brocade/bna/bfa_ioc.c| 8 +- drivers/net/ethernet/brocade/bna/bnad.c | 7 +- drivers/net/ethernet/cadence/macb_main.c | 6 +- drivers/net/ethernet/cadence/macb_pci.c | 2 +- drivers/net/ethernet/calxeda/xgmac.c | 2 + .../ethernet/cavium/liquidio/cn68xx_device.c | 1 + .../net/ethernet/cavium/liquidio/lio_core.c | 92 ++--- .../net/ethernet/cavium/liquidio/lio_main.c | 351 +- .../ethernet/cavium/liquidio/lio_vf_main.c| 158 .../ethernet/cavium/liquidio/octeon_console.c | 12 +- .../ethernet/cavium/liquidio/octeon_device.c | 13 +- .../ethernet/cavium/liquidio/octeon_droq.c| 2 +- .../ethernet/cavium/liquidio/octeon_mailbox.c | 5 +- .../ethernet/cavium/liquidio/octeon_mem_ops.c | 1 + .../net/ethernet/chelsio/cxgb3/cxgb3_main.c | 8 +- drivers/net/ethernet/chelsio/cxgb3/sge.c | 28 +- drivers/net/ethernet/chelsio/cxgb3/t3_hw.c| 5 +- drivers/net/ethernet/cisco/enic/enic_api.c| 2 +- .../net/ethernet/cisco/enic/enic_ethtool.c| 2 +- drivers/net/ethernet/cortina/gemini.c | 8 +- drivers/net/ethernet/dec/tulip/de4x5.c| 4 +- drivers/net/ethernet/dec/tulip/media.c| 5 - drivers/net/ethernet/dnet.c | 8 +- drivers/net/ethernet/ethoc.c | 6 +- .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- drivers/net/ethernet/freescale/fec_ptp.c | 8 +- drivers/net/ethernet/freescale/fman/fman.c| 14 +- .../net/ethernet/freescale/fman/fman_muram.c | 6 +- .../net/ethernet/freescale/fman/fman_port.c | 23 +- drivers/net/ethernet/freescale/fman/mac.c | 4 +- drivers/net/ethernet/hisilicon/hns/hnae.c | 2 +- .../net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 34 +- .../ethernet/hisilicon/hns/hns_dsaf_main.c| 148 .../ethernet/hisilicon/hns/hns_dsaf_misc.c| 7 +- .../net/ethernet/hisilicon/hns/hns_dsaf_ppe.c | 17 +- .../net/ethernet/hisilicon/hns/hns_dsaf_rcb.c | 7 +- .../ethernet/hisilicon/hns/hns_dsaf_xgmac.c | 3 +- drivers/net/ethernet/hisilicon/hns/hns_enet.c | 4 +- .../net/ethernet/hisilicon/hns/hns_ethtool.c | 48 +-- .../hisilicon/hns3/hns3pf/hclge_dcb.c | 1 + drivers/net/ethernet/hisilicon/hns_mdio.c | 3 +- .../net/ethernet/huawei/hinic/hinic_hw_cmdq.c | 2 +- .../net/ethernet/huawei/hinic/hinic_hw_dev.c | 6 +- .../net/ethernet/hu
[PATCH net-next v2 01/10] i40e: prepare flash string in a simpler way
The flash string handling code triggered a W=1 warning and upon
investigation it seems everything can be handled in a simpler
way with a single initialization and one strlcat. The buffer is filled
with NULL after the end of the string by the initializer, and the
strlcat checks total length, and makes sure the buffer is terminated
cleanly.
I didn't mark this with a fixes tag as there is no apparent bug since
the existing code would limit the input data + path to be the right
size, but it does fix the W=1 warning.
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/i40e/i40e_ddp.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
index 5e08f100c413..9767fdf56124 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
@@ -434,14 +434,10 @@ int i40e_ddp_flash(struct net_device *netdev, struct
ethtool_flash *flash)
* stored profile.
*/
if (strncmp(flash->data, "-", 2) != 0) {
+ char profile_name[ETHTOOL_FLASH_MAX_FILENAME] =
I40E_DDP_PROFILE_PATH;
struct i40e_ddp_old_profile_list *list_entry;
- char profile_name[sizeof(I40E_DDP_PROFILE_PATH)
- + I40E_DDP_PROFILE_NAME_MAX];
- profile_name[sizeof(profile_name) - 1] = 0;
- strncpy(profile_name, I40E_DDP_PROFILE_PATH,
- sizeof(profile_name) - 1);
- strncat(profile_name, flash->data, I40E_DDP_PROFILE_NAME_MAX);
+ strlcat(profile_name, flash->data, ETHTOOL_FLASH_MAX_FILENAME);
/* Load DDP recipe. */
status = request_firmware(&ddp_config, profile_name,
&netdev->dev);
--
2.28.0
[PATCH net-next v2 09/10] sfc: fix kdoc warning
kernel-doc script as used by W=1, is confused by the macro
usage inside the header describing the efx_ptp_data struct.
drivers/net/ethernet/sfc/ptp.c:345: warning: Function parameter or member
'MC_CMD_PTP_IN_TRANSMIT_LENMAX' not described in 'efx_ptp_data'
After some discussion on the list, break this patch out to
a separate one, and fix the issue through a creative
macro declaration.
Signed-off-by: Jesse Brandeburg
Cc: Edward Cree
---
drivers/net/ethernet/sfc/mcdi.h | 1 +
drivers/net/ethernet/sfc/ptp.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index ef6d21e4bd0b..69c2924a147c 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -190,6 +190,7 @@ void efx_mcdi_sensor_event(struct efx_nic *efx, efx_qword_t
*ev);
* 32-bit-aligned. Also, on Siena we must copy to the MC shared
* memory strictly 32 bits at a time, so add any necessary padding.
*/
+#define MCDI_TX_BUF_LEN(_len) DIV_ROUND_UP((_len), 4)
#define _MCDI_DECLARE_BUF(_name, _len) \
efx_dword_t _name[DIV_ROUND_UP(_len, 4)]
#define MCDI_DECLARE_BUF(_name, _len) \
diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index bea4725a4499..a5f0c943a9bf 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -325,7 +325,7 @@ struct efx_ptp_data {
struct work_struct pps_work;
struct workqueue_struct *pps_workwq;
bool nic_ts_enabled;
- _MCDI_DECLARE_BUF(txbuf, MC_CMD_PTP_IN_TRANSMIT_LENMAX);
+ efx_dword_t txbuf[MCDI_TX_BUF_LEN(MC_CMD_PTP_IN_TRANSMIT_LENMAX)];
unsigned int good_syncs;
unsigned int fast_syncs;
--
2.28.0
Re: [PATCH][next][v2] iavf: use kvzalloc instead of kzalloc for rx/tx_bi buffer
Li RongQing wrote: > when changes the rx/tx ring to 4096, kzalloc may fail due to > a temporary shortage on slab entries. > > so using kvmalloc to allocate this memory as there is no need > that this memory area is physical continuously. > > and using __GFP_RETRY_MAYFAIL to allocate from kmalloc as > far as possible, which can reduce TLB pressure than vmalloc > as suggested by Eric Dumazet This change is no good, it's using RETRY_MAYFAIL but still using v*alloc. Please see my replies to the i40e version of the patch. I don't think we should apply these.
Re: [PATCH][next] i40e: switch kvzalloc to allocate rx/tx_bi buffer
Li RongQing wrote: > when changes the rx/tx ring to 4096, rx/tx_bi needs an order > 5 pages, and allocation maybe fail due to memory fragmentation > so switch to kvzalloc Hi Li, Thanks for your patches, but I think you're solving a problem that isn't a problem (besides that the warning is being printed.) After all, the driver either gets the memory that it needed via the kernel waiting (since we didn't set GFP_NOWAIT), or ENOMEM gets returned to the user. If your kernel is so close to OOM that it's having this problem aren't you going to have other issues? Anyway This driver goes to great lengths to not make any changes to the existing queues if an allocation fails via ethtool (in fact this is code I authored). Maybe a better option is to just set __GFP_NOWARN on the kcalloc call? Then if there is a memory allocation error we'll just reflect it to user space and let the user retry, with no noisy kernel message. For performance reasons, having actually contiguous regions from kcalloc should help performance vs kvcalloc virtually contiguous regions. I'd prefer that you don't solve the problem this way. How about just marking the allocations as GFP_RETRY_MAYFAIL and GFP_NOWARN? The same goes for the iavf patch. Jesse
Re: [PATCH bpf v4] xsk: do not discard packet when NETDEV_TX_BUSY
Magnus Karlsson wrote:
> From: Magnus Karlsson
>
> In the skb Tx path, transmission of a packet is performed with
> dev_direct_xmit(). When NETDEV_TX_BUSY is set in the drivers, it
> signifies that it was not possible to send the packet right now,
> please try later. Unfortunately, the xsk transmit code discarded the
> packet and returned EBUSY to the application. Fix this unnecessary
> packet loss, by not discarding the packet in the Tx ring and return
> EAGAIN. As EAGAIN is returned to the application, it can then retry
> the send operation later and the packet will then likely be sent as
> the driver will then likely have space/resources to send the packet.
>
> In summary, EAGAIN tells the application that the packet was not
> discarded from the Tx ring and that it needs to call send()
> again. EBUSY, on the other hand, signifies that the packet was not
> sent and discarded from the Tx ring. The application needs to put the
> packet on the Tx ring again if it wants it to be sent.
>
> Fixes: 35fcde7f8deb ("xsk: support for Tx")
> Signed-off-by: Magnus Karlsson
> Reported-by: Arkadiusz Zema
> Suggested-by: Arkadiusz Zema
> Suggested-by: Daniel Borkmann
Patch seems to make sense to me, better matches the expectations/path
of the stack.
Reviewed-by: Jesse Brandeburg
Re: [RFC PATCH net-next v1 11/11] drivers/net/ethernet: clean up mis-targeted comments
Edward Cree wrote: > On 11/09/2020 23:26, Jakub Kicinski wrote: > > #declare _MCDI_BUF_LEN(_len) DIV_ROUND_UP(_len, 4) > > > > efx_dword_t txbuf[_MCDI_BUF_LEN(MC_CMD_PTP_IN_TRANSMIT_LENMAX)]; > > > > Would that work? > That could work, yes. Though, probably lose the leading underscore > in that case. Ok, I made a split-out patch for that change in v2, it seems to work once I found a name that didn't collide. Thanks for the useful discussion!
Re: [Intel-wired-lan] [RFC PATCH net-next v1 05/11] intel-ethernet: make W=1 build cleanly
Vinicius Costa Gomes wrote:
> > diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> > b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> > index 4e7a0810eaeb..2120dacfd55c 100644
> > --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> > +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> > @@ -139,6 +139,7 @@ static void e1000_phy_init_script(struct e1000_hw *hw)
> > * at the end of this routine.
> > */
> > ret_val = e1000_read_phy_reg(hw, 0x2F5B, &phy_saved_data);
> > + e_dbg("Reading PHY register 0x2F5B failed: %d\n", ret_val);
> >
>
> Adding this debug statement seems unrelated.
Thanks, in the next version I actually addressed this in the commit
message, that this one change was to solve the "you didn't use ret_val"
with a conceivably useful message. I also rejiggered the patches to
have the register read lvalue removals all in their own patch instead
of squashed together with kdoc changes.
>
> > /* Disabled the PHY transmitter */
> > e1000_write_phy_reg(hw, 0x2F5B, 0x0003);
>
> Apart from this,
>
> Reviewed-by: Vinicius Costa Gomes
>
Thanks for the review!
Re: [RFC PATCH net-next v1 11/11] drivers/net/ethernet: clean up mis-targeted comments
Edward Cree wrote: > On 11/09/2020 02:23, Jesse Brandeburg wrote: > > + * @MC_CMD_PTP_IN_TRANSMIT_LENMAX: hack to get W=1 to compile > I think I'd rather have a bogus warning than bogus kerneldocto suppress it; > please drop this line (and encourage toolchain folks to figure out how to > get kerneldoc to ignore macros it can't understand). > Apart from that, the sfc and sfc/falcon parts LGTM. > > -ed Thanks Ed, I think I might just remove the /** on that function then (removing it from kdoc processing), would that be acceptable in the meantime? Of course I'd remove the line I added as well. - Jesse
Re: [RFC PATCH net-next v1 00/11] make drivers/net/ethernet W=1 clean
Jakub Kicinski wrote: > Yeah, maybe you need COMPILE_TEST? (full list of the warnings triggered > by the last patch at the end of the email) Ah, good idea, I checked and I have that set, however, I understand what's going on now. > > but I'd like to target the actual job you're running and use that as > > the short-term goal. > > If the code is of any use: > > https://github.com/kuba-moo/nipa > > But it expects to run against a patchwork instance. Thanks! that's interesting on it's own! > > > Also, if you have any comments about the removal of the lvalue from > > some of the register read operations, I figure that is the riskiest > > part of all this. > > Nothing looked suspicious to me. Besides if the compiler is warning that > the variable is unused I'm pretty sure it will optimize that variable > out, anyway so machine code shouldn't change with this series. Good point, I'll check that. > ../drivers/net/ethernet/atheros/atlx/atl1.c:1999:26: warning: cast to > restricted __le16 > ../drivers/net/ethernet/atheros/atlx/atl1.c:2060:33: warning: cast to > restricted __le16 > ../drivers/net/ethernet/atheros/atlx/atl1.c:2125:45: warning: invalid > assignment: |= > ../drivers/net/ethernet/atheros/atlx/atl1.c:2125:45:left side has type > restricted __le32 If I'm not mistaken *all* the warnings you had listed are from C=1 (sparse) which would be best fixed with a second set of patches. This set of patches only aimed to get rid of the W=1 (gcc warnings and kdoc warnings from scripts/kernel-doc) If you run the commands separately you'll see what I mean. make W=1 M=drivers/net/ethernet make C=2 M=drivers/net/ethernet C=2 will force the sparse check but unfortunately will also rebuild everything again. I see about 1188 unique sparse warnings in drivers/net/ethernet and only 16 unique errors from sparse. It's a lot but fixable. However my experience with these warnings is that I could break something in fixing them on drivers I can't test. I just did some spelunking on the sparse warnings, there are only 82 different ones (many are repeated). About half are context imbalances where a lock maybe isn't released. I would bet a bunch are undetected bugs. TL;DR Here is a list of driver files with sparse warnings from C=1, maybe we can encourage some others to help me fix them? drivers/net/ethernet/3com/3c509.c drivers/net/ethernet/3com/3c574_cs.c drivers/net/ethernet/3com/3c589_cs.c drivers/net/ethernet/3com/3c59x.c drivers/net/ethernet/3com/typhoon.c drivers/net/ethernet/8390/axnet_cs.c drivers/net/ethernet/8390/ne2k-pci.c drivers/net/ethernet/8390/pcnet_cs.c drivers/net/ethernet/adaptec/starfire.c drivers/net/ethernet/alacritech/slicoss.c drivers/net/ethernet/alteon/acenic.c drivers/net/ethernet/amd/pcnet32.c drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c drivers/net/ethernet/arc/emac_main.c drivers/net/ethernet/atheros/alx/main.c drivers/net/ethernet/atheros/atl1c/atl1c_hw.c drivers/net/ethernet/atheros/atl1c/atl1c_main.c drivers/net/ethernet/atheros/atl1e/atl1e_main.c drivers/net/ethernet/atheros/atlx/atl1.c drivers/net/ethernet/broadcom/bnx2.c drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c drivers/net/ethernet/broadcom/cnic.c drivers/net/ethernet/broadcom/tg3.c drivers/net/ethernet/brocade/bna/bfa_cee.c drivers/net/ethernet/brocade/bna/bfa_ioc.c drivers/net/ethernet/brocade/bna/bfa_ioc.h drivers/net/ethernet/brocade/bna/bfa_msgq.c drivers/net/ethernet/brocade/bna/bnad.c drivers/net/ethernet/brocade/bna/bna_enet.c drivers/net/ethernet/brocade/bna/bna_tx_rx.c drivers/net/ethernet/cadence/macb_main.c drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c drivers/net/ethernet/cavium/liquidio/cn23xx_vf_device.c drivers/net/ethernet/cavium/liquidio/lio_core.c drivers/net/ethernet/cavium/liquidio/lio_main.c drivers/net/ethernet/cavium/liquidio/lio_vf_main.c drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c drivers/net/ethernet/cavium/liquidio/request_manager.c drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c drivers/net/ethernet/chelsio/cxgb3/sge.c drivers/net/ethernet/chelsio/cxgb3/t3_hw.c drivers/net/ethernet/chelsio/cxgb4vf/sge.c drivers/net/ethernet/chelsio/cxgb/sge.c drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c drivers/net/ethernet/cisco/enic/enic_ethtool.c drivers/net/ethernet/cisco/enic/enic_main.c drivers/net/ethernet/cisco/enic/enic_pp.c drivers/net/ethernet/cisco/enic/vnic_vic.c drivers/net/ethernet/dlink/dl2k.c drivers/net/ethernet/dlink/sundance.c drivers/net/ethernet/emulex/benet/be_cmds.c drivers/net/ethernet/emulex/benet/be_main.c drivers/net/ethernet/ethoc.c drivers/net/ethernet/freescale/dpaa2/dpmac.c drivers/net/ethernet/freescale/enetc/enetc_ethtool.c drivers/net/ethernet/freescale/enetc/enetc_hw.h drivers/net/ethernet/freescale/enetc/enetc_qos.c drivers/net/ethernet/freescale/fsl_pq_mdio.c dri
Re: [RFC PATCH net-next v1 00/11] make drivers/net/ethernet W=1 clean
Jakub Kicinski wrote: > On Thu, 10 Sep 2020 18:23:26 -0700 Jesse Brandeburg wrote: > > Some of these patches are already sent to Intel Wired Lan, but the rest > > of the series titled drivers/net/ethernet affects other drivers, not > > just Intel, but they depend on the first five. > > Great stuff. Much easier to apply one large series than a thousand > small patches. I haven't read all the comment changes but FWIW: > > Reviewed-by: Jakub Kicinski Thanks! > I feel slightly bad for saying this but I think your config did not > include all the drivers, 'cause I'm still getting some warnings after > patch 11. Regardless this is impressive effort, thanks! No worries! I want to get it right, can you share your methodology? I saw from some other message that you're doing make CC="ccache gcc" allmodconfig make CC="ccache gcc" -j 64 W=1 C=1 Is that the right sequence? did you start with a make mrproper as well? I may have missed some drivers when I did this: make allyesconfig make menuconfig but I'd like to target the actual job you're running and use that as the short-term goal. Also, if you have any comments about the removal of the lvalue from some of the register read operations, I figure that is the riskiest part of all this.
[RFC PATCH net-next v1 04/11] ixgbe: clean up W=1 warnings in ixgbe
Inspired by Lee Jones, this eliminates the remaining W=1 warnings in
ixgbe.
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 ++-
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 8
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 0b675c34ce49..02899f79f0ff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6175,8 +6175,9 @@ static void ixgbe_set_eee_capable(struct ixgbe_adapter
*adapter)
/**
* ixgbe_tx_timeout - Respond to a Tx Hang
* @netdev: network interface device structure
+ * @txqueue: queue number that timed out
**/
-static void ixgbe_tx_timeout(struct net_device *netdev, unsigned int txqueue)
+static void ixgbe_tx_timeout(struct net_device *netdev, unsigned int
__always_unused txqueue)
{
struct ixgbe_adapter *adapter = netdev_priv(netdev);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index 7980d7265e10..f77fa3e4fdd1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -771,7 +771,7 @@ static s32 ixgbe_mii_bus_write_generic(struct ixgbe_hw *hw,
int addr,
/**
* ixgbe_mii_bus_read - Read a clause 22/45 register
- * @hw: pointer to hardware structure
+ * @bus: pointer to mii_bus structure which points to our driver private
* @addr: address
* @regnum: register number
**/
@@ -786,7 +786,7 @@ static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int
addr, int regnum)
/**
* ixgbe_mii_bus_write - Write a clause 22/45 register
- * @hw: pointer to hardware structure
+ * @bus: pointer to mii_bus structure which points to our driver private
* @addr: address
* @regnum: register number
* @val: value to write
@@ -803,7 +803,7 @@ static s32 ixgbe_mii_bus_write(struct mii_bus *bus, int
addr, int regnum,
/**
* ixgbe_x550em_a_mii_bus_read - Read a clause 22/45 register on x550em_a
- * @hw: pointer to hardware structure
+ * @bus: pointer to mii_bus structure which points to our driver private
* @addr: address
* @regnum: register number
**/
@@ -820,7 +820,7 @@ static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus,
int addr,
/**
* ixgbe_x550em_a_mii_bus_write - Write a clause 22/45 register on x550em_a
- * @hw: pointer to hardware structure
+ * @bus: pointer to mii_bus structure which points to our driver private
* @addr: address
* @regnum: register number
* @val: value to write
--
2.27.0
[RFC PATCH net-next v1 03/11] iavf: clean up W=1 warnings in iavf
Inspired by Lee Jones, this eliminates the remaining W=1 warnings
in iavf.
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 24 ++---
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
b/drivers/net/ethernet/intel/iavf/iavf_main.c
index d870343cf689..814e59bf2c94 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -147,6 +147,7 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
/**
* iavf_tx_timeout - Respond to a Tx Hang
* @netdev: network interface device structure
+ * @txqueue: queue number that is timing out
**/
static void iavf_tx_timeout(struct net_device *netdev, unsigned int txqueue)
{
@@ -2572,8 +2573,8 @@ static int iavf_validate_ch_config(struct iavf_adapter
*adapter,
}
/**
- * iavf_del_all_cloud_filters - delete all cloud filters
- * on the traffic classes
+ * iavf_del_all_cloud_filters - delete all cloud filters on the traffic classes
+ * @adapter: board private structure
**/
static void iavf_del_all_cloud_filters(struct iavf_adapter *adapter)
{
@@ -2592,7 +2593,7 @@ static void iavf_del_all_cloud_filters(struct
iavf_adapter *adapter)
/**
* __iavf_setup_tc - configure multiple traffic classes
* @netdev: network interface device structure
- * @type_date: tc offload data
+ * @type_data: tc offload data
*
* This function processes the config information provided by the
* user to configure traffic classes/queue channels and packages the
@@ -2690,7 +2691,7 @@ static int __iavf_setup_tc(struct net_device *netdev,
void *type_data)
/**
* iavf_parse_cls_flower - Parse tc flower filters provided by kernel
* @adapter: board private structure
- * @cls_flower: pointer to struct flow_cls_offload
+ * @f: pointer to struct flow_cls_offload
* @filter: pointer to cloud filter structure
*/
static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
@@ -3064,8 +3065,8 @@ static int iavf_delete_clsflower(struct iavf_adapter
*adapter,
/**
* iavf_setup_tc_cls_flower - flower classifier offloads
- * @netdev: net device to configure
- * @type_data: offload data
+ * @adapter: board private structure
+ * @cls_flower: pointer to flow_cls_offload struct with flow info
*/
static int iavf_setup_tc_cls_flower(struct iavf_adapter *adapter,
struct flow_cls_offload *cls_flower)
@@ -3112,7 +3113,7 @@ static LIST_HEAD(iavf_block_cb_list);
* iavf_setup_tc - configure multiple traffic classes
* @netdev: network interface device structure
* @type: type of offload
- * @type_date: tc offload data
+ * @type_data: tc offload data
*
* This function is the callback to ndo_setup_tc in the
* netdev_ops.
@@ -3768,8 +3769,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
/**
* iavf_suspend - Power management suspend routine
- * @pdev: PCI device information struct
- * @state: unused
+ * @dev_d: device info pointer
*
* Called when the system (VM) is entering sleep/suspend.
**/
@@ -3799,15 +3799,15 @@ static int __maybe_unused iavf_suspend(struct device
*dev_d)
/**
* iavf_resume - Power management resume routine
- * @pdev: PCI device information struct
+ * @dev_d: device info pointer
*
* Called when the system (VM) is resumed from sleep/suspend.
**/
static int __maybe_unused iavf_resume(struct device *dev_d)
{
+ struct net_device *netdev = dev_get_drvdata(dev_d);
+ struct iavf_adapter *adapter = netdev_priv(netdev);
struct pci_dev *pdev = to_pci_dev(dev_d);
- struct iavf_adapter *adapter = pci_get_drvdata(pdev);
- struct net_device *netdev = adapter->netdev;
u32 err;
pci_set_master(pdev);
--
2.27.0
