Re: [PATCH net] bnxt_en: fix error return code in bnxt_init_board()

[Michael Chan]

On Thu, Nov 19, 2020 at 9:53 PM Jakub Kicinski  wrote:
>
> On Thu, 19 Nov 2020 10:53:23 -0800 Edwin Peer wrote:
> > > Fix to return a negative error code from the error handling
> > > case instead of 0, as done elsewhere in this function.
> > >
> > > Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> > > Reported-by: Hulk Robot 
> > > Signed-off-by: Zhang Changzhong 
>
> > > if (dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64)) != 0 
> > > &&
> > > dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32)) != 0) 
> > > {
> > > dev_err(>dev, "System does not support DMA, 
> > > aborting\n");
> > > +   rc = -EIO;
> > > goto init_err_disable;
>
> Edwin, please double check if this shouldn't jump to
> pci_release_regions() (or maybe it's harmless 'cause
> PCI likes to magically release things on its own).

Good point.  We definitely should call pci_release_regions() for correctness.

I will send out the patch shortly.  Thanks.


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH net] net: b44: fix error return code in b44_init_one()

[Michael Chan]

On Mon, Nov 16, 2020 at 7:01 PM 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: 39a6f4bce6b4 ("b44: replace the ssb_dma API with the generic DMA API")
> Reported-by: Hulk Robot 
> Signed-off-by: Zhang Changzhong 

Reviewed-by: Michael Chan 

Thanks.


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH net-next RFC v4 01/15] devlink: Add reload action option to devlink reload command

[Michael Chan]

On Mon, Sep 14, 2020 at 2:31 PM Jakub Kicinski  wrote:
>
> On Mon, 14 Sep 2020 13:28:29 +0200 Jiri Pirko wrote:
> > >> Instead, why don't you block in reload_up() until the reset is complete?
> > >
> > >Though user initiate "devlink dev reload" event on a single interface,
> > >all driver entities undergo reset and all entities recover
> > >independently. I don't think we can block the reload_up() on the
> > >interface(that user initiated the command), until whole reset is
> > >complete.
> >
> > Why not? mlxsw reset takes up to like 10 seconds for example.
>
> +1, why?

Yes, we should be able to block until the reset sequence is complete.
I don't see any problem.  I will work with Vasundhara on this.

Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes

[Michael Chan]

On Fri, Jul 24, 2020 at 5:19 PM David Christensen
 wrote:

> In the working case, tg3_init_hw() returns successfully, resulting in
> every instance of napi_disable() being followed by an instance of
> napi_enable().
>
> In the failing case, tg3_hw_init() returns an error. (This is not
> surprising since the system is now preventing the adapter from accessing
> its MMIO registers.  I'm curious why it doesn't always fail.)  When
> tg3_hw_init() fails, tg3_netif_start() is not called, and we end up with
> two sequential calls to napi_disable(), resulting in multiple hung task
> messages.
>

If the driver fails to initialize the chip completely, the tg3_flags
should indicate we are in this failed state.  We already have
TG3_FLAG_INIT_COMPLETE.  Perhaps, we can expand the use of this flag
to cover the scenario that you described above.  We can clear
TG3_FLAG_INIT_COMPLETE before calling tg3_halt() and only set it back
when tg3_hw_init() completes successfully.  This is the rough idea,
but a more detailed analysis on how this flag is used needs to be done
first.

Assuming this works, the EEH handler can check TG3_FLAG_INIT_COMPLETE
to see if we should call tg3_netif_stop().

Another way to fix it is to call dev_close() if tg3_reset_task() fails
to re-initialize the device.

Re: [PATCH][next] tg3: Avoid the use of one-element array

[Michael Chan]

On Wed, Jul 22, 2020 at 11:38 AM Gustavo A. R. Silva
 wrote:
>
> One-element arrays are being deprecated[1]. Replace the one-element
> array with a simple value type 'u32 reserved2'[2], once it seems
> this is just a placeholder for alignment.
>
> [1] https://github.com/KSPP/linux/issues/79
> [2] https://github.com/KSPP/linux/issues/86
>
> Tested-by: kernel test robot 
> Link: 
> https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/0-day/tg3-20200718.md
> Signed-off-by: Gustavo A. R. Silva 

Reviewed-by: Michael Chan 

Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes

[Michael Chan]

On Wed, Jun 17, 2020 at 11:51 AM David Christensen
 wrote:
>
> The driver function tg3_io_error_detected() calls napi_disable twice,
> without an intervening napi_enable, when the number of EEH errors exceeds
> eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock.
>
> Add check for pcierr_recovery which skips code already executed for the
> "Frozen" state.
>
> Signed-off-by: David Christensen 

Reviewed-by: Michael Chan 

Thanks.

Re: [PATCH] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes

[Michael Chan]

On Mon, Jun 15, 2020 at 3:21 PM David Christensen
 wrote:
>
> On 6/15/20 1:45 PM, Michael Chan wrote:
> > On Mon, Jun 15, 2020 at 12:01 PM David Christensen
> >  wrote:
> >>
> >> The driver function tg3_io_error_detected() calls napi_disable twice,
> >> without an intervening napi_enable, when the number of EEH errors exceeds
> >> eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock.
> >>
> >> The function is called once with the PCI state pci_channel_io_frozen and
> >> then called again with the state pci_channel_io_perm_failure when the
> >> number of EEH failures in an hour exceeds eeh_max_freezes.
> >>
> >> Protecting the calls to napi_enable/napi_disable with a new state
> >> variable prevents the long sleep.
> >
> > This works, but I think a simpler fix is to check tp->pcierr_recovery
> > in tg3_io_error_detected() and skip most of the tg3 calls (including
> > the one that disables NAPI) if the flag is true.
>
> This might be the smallest change that would work.  Does it make sense
> to the reader?
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c
> b/drivers/net/ethernet/broadcom/tg3.c
> index 7a3b22b35238..1f37c69d213d 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -18168,8 +18168,8 @@ static pci_ers_result_t
> tg3_io_error_detected(struct pci_dev *pdev,
>
>  rtnl_lock();
>
> -   /* We probably don't have netdev yet */
> -   if (!netdev || !netif_running(netdev))
> +   /* May be second call or maybe we don't have netdev yet */
> +   if (tp->pcierr_recovery || !netdev || !netif_running(netdev))

Dereferencing tp needs to be done after checking netdev.  If we don't
have netdev, tp won't be valid.

Other than that, I think the logic looks good and is quite clear.

Thanks.

Re: [PATCH] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes

[Michael Chan]

On Mon, Jun 15, 2020 at 12:01 PM David Christensen
 wrote:
>
> The driver function tg3_io_error_detected() calls napi_disable twice,
> without an intervening napi_enable, when the number of EEH errors exceeds
> eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock.
>
> The function is called once with the PCI state pci_channel_io_frozen and
> then called again with the state pci_channel_io_perm_failure when the
> number of EEH failures in an hour exceeds eeh_max_freezes.
>
> Protecting the calls to napi_enable/napi_disable with a new state
> variable prevents the long sleep.

This works, but I think a simpler fix is to check tp->pcierr_recovery
in tg3_io_error_detected() and skip most of the tg3 calls (including
the one that disables NAPI) if the flag is true.

Re: [PATCH v3 6/8] bnxt_en: use new taint_firmware_crashed()

[Michael Chan]

On Tue, May 26, 2020 at 7:58 AM Luis Chamberlain  wrote:
>
> From: Vasundhara Volam 
>
> This makes use of the new module_firmware_crashed() to help
> annotate when firmware for device drivers crash. When firmware
> crashes devices can sometimes become unresponsive, and recovery
> sometimes requires a driver unload / reload and in the worst cases
> a reboot.
>
> Using a taint flag allows us to annotate when this happens clearly.
>
> Cc: Michael Chan 
> Cc: Luis Chamberlain 
> Acked-by: Rafael Aquini 
> Signed-off-by: Vasundhara Volam 
> Signed-off-by: Luis Chamberlain 

Reviewed-by: Michael Chan 

Re: [PATCH] cnic: remove redundant assignment to variable ret

[Michael Chan]

On Fri, May 8, 2020 at 3:40 PM Colin King  wrote:
>
> From: Colin Ian King 
>
> The variable ret is being assigned with a value that is never read,
> the assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King 

Reviewed-by: Michael Chan 

Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

[Michael Chan]

On Fri, May 8, 2020 at 7:31 PM Joe Perches  wrote:
>
> On Fri, 2020-05-08 at 18:48 -0700, Jakub Kicinski wrote:
> > On Sat, 9 May 2020 00:31:03 +0100 Colin Ian King wrote:
> > > > My preference would be for
> > > >
> > > > {
> > > >   int i;
> > > >   u32 off = 0;
> > > >
> > > >   for (i = 0; i < TG3_SD_NUM_RECS; i++) {
> > > >   tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);
> > > >
> > > >   if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> > > >   !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> > > >   memset(ocir, 0, TG3_OCIR_LEN);
> > > >
> > > >   off += TG3_OCIR_LEN;
> > > >   ocir++;
> > > >   }
> > > >
> > > OK, I'll send a V3 tomorrow.
> >
> > I already reviewed and applied v2, just waiting for builds to finish,
> > let's leave it.
>
>
> I think clarity should be preferred.

Either way is fine with me.  I'm fine with v2 since it's already applied.

Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

[Michael Chan]

On Fri, May 8, 2020 at 3:53 PM Colin King  wrote:
>
> From: Colin Ian King 
>
> Currently the value for 'off' is computed using a multiplication and
> a couple of statements later off is being incremented by len and
> this value is never read.  Clean up the code by removing the
> multiplication and just increment off by len on each iteration. Also
> use len instead of TG3_OCIR_LEN.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/ethernet/broadcom/tg3.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> b/drivers/net/ethernet/broadcom/tg3.c
> index ff98a82b7bc4..9dd9bd506bcc 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -10798,16 +10798,14 @@ static int tg3_init_hw(struct tg3 *tp, bool 
> reset_phy)
>  static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
>  {
> int i;
> +   u32 off, len = TG3_OCIR_LEN;

Please use reverse X-mas tree style variable declarations.  Other than
that, it looks good.

Thanks.

>
> -   for (i = 0; i < TG3_SD_NUM_RECS; i++, ocir++) {
> -   u32 off = i * TG3_OCIR_LEN, len = TG3_OCIR_LEN;
> -
> +   for (i = 0, off = 0; i < TG3_SD_NUM_RECS; i++, ocir++, off += len) {
> tg3_ape_scratchpad_read(tp, (u32 *) ocir, off, len);
> -   off += len;
>
> if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> -   memset(ocir, 0, TG3_OCIR_LEN);
> +   memset(ocir, 0, len);
> }
>  }
>
> --
> 2.25.1
>

Re: [PATCH 2/2] cnic: Use refcount_t for refcount

[Michael Chan]

On Wed, Jul 31, 2019 at 7:22 PM Chuhong Yuan  wrote:
>
> Michael Chan  于2019年8月1日周四 上午1:58写道：
> >
> > On Wed, Jul 31, 2019 at 5:22 AM Chuhong Yuan  wrote:
> >
> > >  static void cnic_ctx_wr(struct cnic_dev *dev, u32 cid_addr, u32 off, u32 
> > > val)
> > > @@ -494,7 +494,7 @@ int cnic_register_driver(int ulp_type, struct 
> > > cnic_ulp_ops *ulp_ops)
> > > }
> > > read_unlock(_dev_lock);
> > >
> > > -   atomic_set(_ops->ref_count, 0);
> > > +   refcount_set(_ops->ref_count, 0);
> > > rcu_assign_pointer(cnic_ulp_tbl[ulp_type], ulp_ops);
> > > mutex_unlock(_lock);
> > >
> >
> > Willem's comment applies here too.  The driver needs to be modified to
> > count from 1 to use refcount_t.
> >
> > Thanks.
>
> I have revised this problem but find the other two refcounts -
> cnic_dev::ref_count
> and cnic_sock::ref_count have no set.
> I am not sure where to initialize them to 1.
>
> Besides, should ulp_ops->ref_count be set to 0 when unregistered?

I will send a patch to fix up the initialization of all the atomic ref
counts.  After that, you can add your patch to convert to refcount_t.

Re: [PATCH 2/2] cnic: Use refcount_t for refcount

[Michael Chan]

On Wed, Jul 31, 2019 at 5:22 AM Chuhong Yuan  wrote:

>  static void cnic_ctx_wr(struct cnic_dev *dev, u32 cid_addr, u32 off, u32 val)
> @@ -494,7 +494,7 @@ int cnic_register_driver(int ulp_type, struct 
> cnic_ulp_ops *ulp_ops)
> }
> read_unlock(_dev_lock);
>
> -   atomic_set(_ops->ref_count, 0);
> +   refcount_set(_ops->ref_count, 0);
> rcu_assign_pointer(cnic_ulp_tbl[ulp_type], ulp_ops);
> mutex_unlock(_lock);
>

Willem's comment applies here too.  The driver needs to be modified to
count from 1 to use refcount_t.

Thanks.

Re: [PATCH 1/2] bnxt_en: Use refcount_t for refcount

[Michael Chan]

On Wed, Jul 31, 2019 at 9:06 AM Willem de Bruijn
 wrote:
>
> On Wed, Jul 31, 2019 at 8:22 AM Chuhong Yuan  wrote:
> >
> > refcount_t is better for reference counters since its
> > implementation can prevent overflows.
> > So convert atomic_t ref counters to refcount_t.
> >
> > Signed-off-by: Chuhong Yuan 
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 8 
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c 
> > b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> > index fc77caf0a076..eb7ed34639e2 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> > @@ -49,7 +49,7 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, 
> > int ulp_id,
> > return -ENOMEM;
> > }
> >
> > -   atomic_set(>ref_count, 0);
> > +   refcount_set(>ref_count, 0);
>
> One feature of refcount_t is that it warns on refcount_inc from 0 to
> detect possible use-after_free. It appears that that can trigger here?
>

I think that's right.  We need to change the driver to start counting
from 1 instead of 0 if we convert to refcount.

Re: [PATCH net-next] bnxt: remove printing of hwrm message

[Michael Chan]

On Wed, Dec 12, 2018 at 8:59 AM Jonathan Toppins  wrote:
>
>   bnxt_en :19:00.0 (unregistered net_device) (uninitialized): hwrm
> req_type 0x190 seq id 0x6 error 0x
>
> The message above is commonly seen when a newer driver is used on
> hardware with older firmware. The issue is this message means nothing to
> anyone except Broadcom. Remove the message to not confuse users as this
> message is really not very informative.
>
> Signed-off-by: Jonathan Toppins 
> ---
>
> Notes:
> v2:
>   include changes recommended by Michael Chan
>

Acked-by: Michael Chan 

Re: [PATCH 4.4 51/79] bnxt_en: Fix for system hang if request_irq fails

[Michael Chan]

On Tue, Sep 11, 2018 at 1:14 PM, Ben Hutchings
 wrote:
> On Thu, 2018-08-23 at 09:53 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> --
>>
>> From: Vikas Gupta 
>>
>> [ Upstream commit c58387ab1614f6d7fb9e244f214b61e7631421fc ]
>>
>> Fix bug in the error code path when bnxt_request_irq() returns failure.
>> bnxt_disable_napi() should not be called in this error path because
>> NAPI has not been enabled yet.
> [...]
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -4591,7 +4591,7 @@ static int __bnxt_open_nic(struct bnxt *
>>   rc = bnxt_request_irq(bp);
>>   if (rc) {
>>   netdev_err(bp->dev, "bnxt_request_irq err: %x\n", rc);
>> - goto open_err;
>> + goto open_err_irq;
>>   }
>>   }
>>
>> @@ -4629,6 +4629,8 @@ static int __bnxt_open_nic(struct bnxt *
>>
>>  open_err:
>>   bnxt_disable_napi(bp);
>> +
>> +open_err_irq:
>>   bnxt_del_napi(bp);
>
> Shouldn't this added statement be conditional on irq_re_init?
>

Unconditional call is more correct, because when open fails, we clean
up everything, including the NAPI that was added just now or during a
previous call.

In other words, the NAPI struct is always valid here whether
irq_re_init is true or not.  And we always delete it if open fails.

Thanks.

Re: [PATCH 4.4 51/79] bnxt_en: Fix for system hang if request_irq fails

[Michael Chan]

On Tue, Sep 11, 2018 at 1:14 PM, Ben Hutchings
 wrote:
> On Thu, 2018-08-23 at 09:53 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> --
>>
>> From: Vikas Gupta 
>>
>> [ Upstream commit c58387ab1614f6d7fb9e244f214b61e7631421fc ]
>>
>> Fix bug in the error code path when bnxt_request_irq() returns failure.
>> bnxt_disable_napi() should not be called in this error path because
>> NAPI has not been enabled yet.
> [...]
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -4591,7 +4591,7 @@ static int __bnxt_open_nic(struct bnxt *
>>   rc = bnxt_request_irq(bp);
>>   if (rc) {
>>   netdev_err(bp->dev, "bnxt_request_irq err: %x\n", rc);
>> - goto open_err;
>> + goto open_err_irq;
>>   }
>>   }
>>
>> @@ -4629,6 +4629,8 @@ static int __bnxt_open_nic(struct bnxt *
>>
>>  open_err:
>>   bnxt_disable_napi(bp);
>> +
>> +open_err_irq:
>>   bnxt_del_napi(bp);
>
> Shouldn't this added statement be conditional on irq_re_init?
>

Unconditional call is more correct, because when open fails, we clean
up everything, including the NAPI that was added just now or during a
previous call.

In other words, the NAPI struct is always valid here whether
irq_re_init is true or not.  And we always delete it if open fails.

Thanks.

Re: [PATCH net-next] bnxt_en: Fix logic of forward the VF MAC address to PF in bnxt_vf_validate_set_mac

[Michael Chan]

On Tue, Jul 24, 2018 at 9:01 AM, Vasundhara Volam
 wrote:
> On Tue, Jul 24, 2018 at 1:01 PM, Michael Chan  
> wrote:
>>
>> On Mon, Jul 23, 2018 at 10:24 PM, YueHaibing  wrote:
>> > Based on the comments,req->l2addr must match the VF MAC address
>> > if firmware spec >= 1.2.2, mac_ok can be true.
>> >
>> > Signed-off-by: YueHaibing 
>> > ---
>> >  drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 7 ++-
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c 
>> > b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > index a649108..7925964 100644
>> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > @@ -954,12 +954,9 @@ static int bnxt_vf_validate_set_mac(struct bnxt *bp, 
>> > struct bnxt_vf_info *vf)
>> > if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->mac_addr))
>> > mac_ok = true;
>> > } else if (is_valid_ether_addr(vf->vf_mac_addr)) {
>> > -   if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->vf_mac_addr))
>> > +   if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->vf_mac_addr) &&
>> > +   bp->hwrm_spec_code >= 0x10202)
>> > mac_ok = true;
>>
>> I'm not sure if this is correct.  If firmware spec < 0x10202, the VF
>> MAC address is not forwarded to the PF and so it doesn't have to match
>> and mac_ok should still be true.  I think we are missing that
>> condition with this patch.
>>
>> I need to let my colleague Vasundhara comment on this.  She is more
>> familiar with this logic.
> Yes Michael, you are right. Also, the plain else condition is to cover
> a special case to allow VF to modify
> it's own MAC when PF has not assigned a valid MAC address and HWRM
> spec code > 0x10202.

We should combine the "else if" and "else" below into a plain else and
add some comments to explain the conditions.

>>
>> > -   } else if (bp->hwrm_spec_code < 0x10202) {
>> > -   mac_ok = true;
>> > -   } else {
>> > -   mac_ok = true;
>> > }
>> > if (mac_ok)
>> > return bnxt_hwrm_exec_fwd_resp(bp, vf, msg_size);
>> > --
>> > 2.7.0
>> >
>> >

Re: [PATCH net-next] bnxt_en: Fix logic of forward the VF MAC address to PF in bnxt_vf_validate_set_mac

[Michael Chan]

On Tue, Jul 24, 2018 at 9:01 AM, Vasundhara Volam
 wrote:
> On Tue, Jul 24, 2018 at 1:01 PM, Michael Chan  
> wrote:
>>
>> On Mon, Jul 23, 2018 at 10:24 PM, YueHaibing  wrote:
>> > Based on the comments,req->l2addr must match the VF MAC address
>> > if firmware spec >= 1.2.2, mac_ok can be true.
>> >
>> > Signed-off-by: YueHaibing 
>> > ---
>> >  drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 7 ++-
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c 
>> > b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > index a649108..7925964 100644
>> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
>> > @@ -954,12 +954,9 @@ static int bnxt_vf_validate_set_mac(struct bnxt *bp, 
>> > struct bnxt_vf_info *vf)
>> > if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->mac_addr))
>> > mac_ok = true;
>> > } else if (is_valid_ether_addr(vf->vf_mac_addr)) {
>> > -   if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->vf_mac_addr))
>> > +   if (ether_addr_equal((const u8 *)req->l2_addr, 
>> > vf->vf_mac_addr) &&
>> > +   bp->hwrm_spec_code >= 0x10202)
>> > mac_ok = true;
>>
>> I'm not sure if this is correct.  If firmware spec < 0x10202, the VF
>> MAC address is not forwarded to the PF and so it doesn't have to match
>> and mac_ok should still be true.  I think we are missing that
>> condition with this patch.
>>
>> I need to let my colleague Vasundhara comment on this.  She is more
>> familiar with this logic.
> Yes Michael, you are right. Also, the plain else condition is to cover
> a special case to allow VF to modify
> it's own MAC when PF has not assigned a valid MAC address and HWRM
> spec code > 0x10202.

We should combine the "else if" and "else" below into a plain else and
add some comments to explain the conditions.

>>
>> > -   } else if (bp->hwrm_spec_code < 0x10202) {
>> > -   mac_ok = true;
>> > -   } else {
>> > -   mac_ok = true;
>> > }
>> > if (mac_ok)
>> > return bnxt_hwrm_exec_fwd_resp(bp, vf, msg_size);
>> > --
>> > 2.7.0
>> >
>> >

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Wed, May 2, 2018 at 5:30 PM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
> On 2018年05月03日 01:32, Michael Chan wrote:
>>
>> On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
>>>
>>> On 2018年05月02日 13:12, Michael Chan wrote:
>>>>
>>>> On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen <zumeng.c...@gmail.com>
>>>> wrote:
>>>>
>>>>> diff --git a/drivers/net/ethernet/broadcom/tg3.h
>>>>> b/drivers/net/ethernet/broadcom/tg3.h
>>>>> index 3b5e98e..c61d83c 100644
>>>>> --- a/drivers/net/ethernet/broadcom/tg3.h
>>>>> +++ b/drivers/net/ethernet/broadcom/tg3.h
>>>>> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
>>>>>   TG3_FLAG_ROBOSWITCH,
>>>>>   TG3_FLAG_ONE_DMA_AT_ONCE,
>>>>>   TG3_FLAG_RGMII_MODE,
>>>>> +   TG3_FLAG_HALT,
>>>>
>>>> I think you should be able to use the existing INIT_COMPLETE flag
>>>
>>>
>>> No,  it will bring the uncertain factors into the existed complicate
>>> logic
>>> of INIT_COMPLETE.
>>> And I think it's very simple logic here to fix the meaningless hw_stats
>>> reading and the problem
>>> of commit f5992b72. I even suspect if you have read INIT_COMPLETE related
>>> codes carefully.
>>>
>> We should use an existing flag whenever appropriate
>
>
> I disagree. This is sort of blahblah...
>>

I don't want to see another flag added that is practically the same as
!INIT_COMPLETE.  The driver already has close to one hundred flags.
Adding a new flag that is similar to an existing flag will just make
the code more difficult to understand and maintain.

If you don't want to fix it the cleaner way, Siva or I will fix it.

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Wed, May 2, 2018 at 5:30 PM, Zumeng Chen  wrote:
> On 2018年05月03日 01:32, Michael Chan wrote:
>>
>> On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen  wrote:
>>>
>>> On 2018年05月02日 13:12, Michael Chan wrote:
>>>>
>>>> On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 
>>>> wrote:
>>>>
>>>>> diff --git a/drivers/net/ethernet/broadcom/tg3.h
>>>>> b/drivers/net/ethernet/broadcom/tg3.h
>>>>> index 3b5e98e..c61d83c 100644
>>>>> --- a/drivers/net/ethernet/broadcom/tg3.h
>>>>> +++ b/drivers/net/ethernet/broadcom/tg3.h
>>>>> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
>>>>>   TG3_FLAG_ROBOSWITCH,
>>>>>   TG3_FLAG_ONE_DMA_AT_ONCE,
>>>>>   TG3_FLAG_RGMII_MODE,
>>>>> +   TG3_FLAG_HALT,
>>>>
>>>> I think you should be able to use the existing INIT_COMPLETE flag
>>>
>>>
>>> No,  it will bring the uncertain factors into the existed complicate
>>> logic
>>> of INIT_COMPLETE.
>>> And I think it's very simple logic here to fix the meaningless hw_stats
>>> reading and the problem
>>> of commit f5992b72. I even suspect if you have read INIT_COMPLETE related
>>> codes carefully.
>>>
>> We should use an existing flag whenever appropriate
>
>
> I disagree. This is sort of blahblah...
>>

I don't want to see another flag added that is practically the same as
!INIT_COMPLETE.  The driver already has close to one hundred flags.
Adding a new flag that is similar to an existing flag will just make
the code more difficult to understand and maintain.

If you don't want to fix it the cleaner way, Siva or I will fix it.

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
> On 2018年05月02日 13:12, Michael Chan wrote:
>>
>> On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
>>
>>> diff --git a/drivers/net/ethernet/broadcom/tg3.h
>>> b/drivers/net/ethernet/broadcom/tg3.h
>>> index 3b5e98e..c61d83c 100644
>>> --- a/drivers/net/ethernet/broadcom/tg3.h
>>> +++ b/drivers/net/ethernet/broadcom/tg3.h
>>> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
>>>  TG3_FLAG_ROBOSWITCH,
>>>  TG3_FLAG_ONE_DMA_AT_ONCE,
>>>  TG3_FLAG_RGMII_MODE,
>>> +   TG3_FLAG_HALT,
>>
>> I think you should be able to use the existing INIT_COMPLETE flag
>
>
> No,  it will bring the uncertain factors into the existed complicate logic
> of INIT_COMPLETE.
> And I think it's very simple logic here to fix the meaningless hw_stats
> reading and the problem
> of commit f5992b72. I even suspect if you have read INIT_COMPLETE related
> codes carefully.
>

We should use an existing flag whenever appropriate, instead of adding
yet another flag to do similar things. I've looked at the code briefly
and believe that INIT_COMPLETE will work.  If you think it won't work,
please be specific and point out why it won't work.  Thanks.

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen  wrote:
> On 2018年05月02日 13:12, Michael Chan wrote:
>>
>> On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen  wrote:
>>
>>> diff --git a/drivers/net/ethernet/broadcom/tg3.h
>>> b/drivers/net/ethernet/broadcom/tg3.h
>>> index 3b5e98e..c61d83c 100644
>>> --- a/drivers/net/ethernet/broadcom/tg3.h
>>> +++ b/drivers/net/ethernet/broadcom/tg3.h
>>> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
>>>  TG3_FLAG_ROBOSWITCH,
>>>  TG3_FLAG_ONE_DMA_AT_ONCE,
>>>  TG3_FLAG_RGMII_MODE,
>>> +   TG3_FLAG_HALT,
>>
>> I think you should be able to use the existing INIT_COMPLETE flag
>
>
> No,  it will bring the uncertain factors into the existed complicate logic
> of INIT_COMPLETE.
> And I think it's very simple logic here to fix the meaningless hw_stats
> reading and the problem
> of commit f5992b72. I even suspect if you have read INIT_COMPLETE related
> codes carefully.
>

We should use an existing flag whenever appropriate, instead of adding
yet another flag to do similar things. I've looked at the code briefly
and believe that INIT_COMPLETE will work.  If you think it won't work,
please be specific and point out why it won't work.  Thanks.

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen  wrote:

> diff --git a/drivers/net/ethernet/broadcom/tg3.h 
> b/drivers/net/ethernet/broadcom/tg3.h
> index 3b5e98e..c61d83c 100644
> --- a/drivers/net/ethernet/broadcom/tg3.h
> +++ b/drivers/net/ethernet/broadcom/tg3.h
> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
> TG3_FLAG_ROBOSWITCH,
> TG3_FLAG_ONE_DMA_AT_ONCE,
> TG3_FLAG_RGMII_MODE,
> +   TG3_FLAG_HALT,

I think you should be able to use the existing INIT_COMPLETE flag and
not have to add a new flag.

>
> /* Add new flags before this comment and TG3_FLAG_NUMBER_OF_FLAGS */
> TG3_FLAG_NUMBER_OF_FLAGS,   /* Last entry in enum TG3_FLAGS */
> --
> 2.9.3
>

Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen  wrote:

> diff --git a/drivers/net/ethernet/broadcom/tg3.h 
> b/drivers/net/ethernet/broadcom/tg3.h
> index 3b5e98e..c61d83c 100644
> --- a/drivers/net/ethernet/broadcom/tg3.h
> +++ b/drivers/net/ethernet/broadcom/tg3.h
> @@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
> TG3_FLAG_ROBOSWITCH,
> TG3_FLAG_ONE_DMA_AT_ONCE,
> TG3_FLAG_RGMII_MODE,
> +   TG3_FLAG_HALT,

I think you should be able to use the existing INIT_COMPLETE flag and
not have to add a new flag.

>
> /* Add new flags before this comment and TG3_FLAG_NUMBER_OF_FLAGS */
> TG3_FLAG_NUMBER_OF_FLAGS,   /* Last entry in enum TG3_FLAGS */
> --
> 2.9.3
>

Re: [PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Fri, Apr 27, 2018 at 8:15 PM, Zumeng Chen  wrote:

> diff --git a/drivers/net/ethernet/broadcom/tg3.h 
> b/drivers/net/ethernet/broadcom/tg3.h
> index 3b5e98e..6727d93 100644
> --- a/drivers/net/ethernet/broadcom/tg3.h
> +++ b/drivers/net/ethernet/broadcom/tg3.h
> @@ -3352,6 +3352,7 @@ struct tg3 {
> struct pci_dev  *pdev_peer;
>
> struct tg3_hw_stats *hw_stats;
> +   boolhw_stats_flag;

You can just add another bit to enum TG3_FLAGS for this purpose.

While this scheme will probably work, I think a better and more
elegant way to fix this is to use RCU.

> dma_addr_t  stats_mapping;
> struct work_struct  reset_task;
>
> --
> 2.9.3
>

Re: [PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Michael Chan]

On Fri, Apr 27, 2018 at 8:15 PM, Zumeng Chen  wrote:

> diff --git a/drivers/net/ethernet/broadcom/tg3.h 
> b/drivers/net/ethernet/broadcom/tg3.h
> index 3b5e98e..6727d93 100644
> --- a/drivers/net/ethernet/broadcom/tg3.h
> +++ b/drivers/net/ethernet/broadcom/tg3.h
> @@ -3352,6 +3352,7 @@ struct tg3 {
> struct pci_dev  *pdev_peer;
>
> struct tg3_hw_stats *hw_stats;
> +   boolhw_stats_flag;

You can just add another bit to enum TG3_FLAGS for this purpose.

While this scheme will probably work, I think a better and more
elegant way to fix this is to use RCU.

> dma_addr_t  stats_mapping;
> struct work_struct  reset_task;
>
> --
> 2.9.3
>

Re: [PATCH v7 6/7] bnxt_en: Eliminate duplicate barriers on weakly-ordered archs

[Michael Chan]

On Sun, Mar 25, 2018 at 7:39 AM, Sinan Kaya <ok...@codeaurora.org> wrote:
> Code includes wmb() followed by writel(). writel() already has a barrier on
> some architectures like arm64.
>
> This ends up CPU observing two barriers back to back before executing the
> register write.
>
> Create a new wrapper function with relaxed write operator. Use the new
> wrapper when a write is following a wmb().
>
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
>
> Also add mmiowb() so that write code doesn't move outside of scope.

This line in the patch description is not needed anymore.  Other than that,

Acked-by: Michael Chan <michael.c...@broadcom.com>

Thanks.
>
> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>

Re: [PATCH v7 6/7] bnxt_en: Eliminate duplicate barriers on weakly-ordered archs

[Michael Chan]

On Sun, Mar 25, 2018 at 7:39 AM, Sinan Kaya  wrote:
> Code includes wmb() followed by writel(). writel() already has a barrier on
> some architectures like arm64.
>
> This ends up CPU observing two barriers back to back before executing the
> register write.
>
> Create a new wrapper function with relaxed write operator. Use the new
> wrapper when a write is following a wmb().
>
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
>
> Also add mmiowb() so that write code doesn't move outside of scope.

This line in the patch description is not needed anymore.  Other than that,

Acked-by: Michael Chan 

Thanks.
>
> Signed-off-by: Sinan Kaya 

Re: [PATCH v6 6/6] bnxt_en: Eliminate duplicate barriers on weakly-ordered archs

[Michael Chan]

On Fri, Mar 23, 2018 at 3:23 PM, Sinan Kaya  wrote:
> Code includes wmb() followed by writel(). writel() already has a barrier on
> some architectures like arm64.
>
> This ends up CPU observing two barriers back to back before executing the
> register write.
>
> Create a new wrapper function with relaxed write operator. Use the new
> wrapper when a write is following a wmb().
>
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
>
> Also add mmiowb() so that write code doesn't move outside of scope.
>
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h | 9 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 1500243..fc8ea0d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1922,7 +1922,8 @@ static int bnxt_poll_work(struct bnxt *bp, struct 
> bnxt_napi *bnapi, int budget)
> /* Sync BD data before updating doorbell */
> wmb();
>
> -   bnxt_db_write(bp, db, DB_KEY_TX | prod);
> +   bnxt_db_write_relaxed(bp, db, DB_KEY_TX | prod);
> +   mmiowb();

Sorry for the late review.  mmiowb() is not required here because we
are in NAPI context, so only one CPU will be updating this doorbell.

Other than that, it looks good.

> }
>
> cpr->cp_raw_cons = raw_cons;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 1989c47..5e453b9 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1401,6 +1401,15 @@ static inline u32 bnxt_tx_avail(struct bnxt *bp, 
> struct bnxt_tx_ring_info *txr)
> ((txr->tx_prod - txr->tx_cons) & bp->tx_ring_mask);
>  }
>
> +/* For TX and RX ring doorbells with no ordering guarantee*/
> +static inline void bnxt_db_write_relaxed(struct bnxt *bp, void __iomem *db,
> +u32 val)
> +{
> +   writel_relaxed(val, db);
> +   if (bp->flags & BNXT_FLAG_DOUBLE_DB)
> +   writel_relaxed(val, db);
> +}
> +
>  /* For TX and RX ring doorbells */
>  static inline void bnxt_db_write(struct bnxt *bp, void __iomem *db, u32 val)
>  {
> --
> 2.7.4
>

Re: [PATCH v6 6/6] bnxt_en: Eliminate duplicate barriers on weakly-ordered archs

[Michael Chan]

On Fri, Mar 23, 2018 at 3:23 PM, Sinan Kaya  wrote:
> Code includes wmb() followed by writel(). writel() already has a barrier on
> some architectures like arm64.
>
> This ends up CPU observing two barriers back to back before executing the
> register write.
>
> Create a new wrapper function with relaxed write operator. Use the new
> wrapper when a write is following a wmb().
>
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
>
> Also add mmiowb() so that write code doesn't move outside of scope.
>
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h | 9 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 1500243..fc8ea0d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1922,7 +1922,8 @@ static int bnxt_poll_work(struct bnxt *bp, struct 
> bnxt_napi *bnapi, int budget)
> /* Sync BD data before updating doorbell */
> wmb();
>
> -   bnxt_db_write(bp, db, DB_KEY_TX | prod);
> +   bnxt_db_write_relaxed(bp, db, DB_KEY_TX | prod);
> +   mmiowb();

Sorry for the late review.  mmiowb() is not required here because we
are in NAPI context, so only one CPU will be updating this doorbell.

Other than that, it looks good.

> }
>
> cpr->cp_raw_cons = raw_cons;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 1989c47..5e453b9 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1401,6 +1401,15 @@ static inline u32 bnxt_tx_avail(struct bnxt *bp, 
> struct bnxt_tx_ring_info *txr)
> ((txr->tx_prod - txr->tx_cons) & bp->tx_ring_mask);
>  }
>
> +/* For TX and RX ring doorbells with no ordering guarantee*/
> +static inline void bnxt_db_write_relaxed(struct bnxt *bp, void __iomem *db,
> +u32 val)
> +{
> +   writel_relaxed(val, db);
> +   if (bp->flags & BNXT_FLAG_DOUBLE_DB)
> +   writel_relaxed(val, db);
> +}
> +
>  /* For TX and RX ring doorbells */
>  static inline void bnxt_db_write(struct bnxt *bp, void __iomem *db, u32 val)
>  {
> --
> 2.7.4
>

Re: [PATCH net] tg3: prevent scheduling while atomic splat

[Michael Chan]

On Wed, Mar 14, 2018 at 9:36 AM, Jonathan Toppins <jtopp...@redhat.com> wrote:
> The problem was introduced in commit
> 506b0a395f26 ("[netdrv] tg3: APE heartbeat changes"). The bug occurs
> because tp->lock spinlock is held which is obtained in tg3_start
> by way of tg3_full_lock(), line 11571. The documentation for usleep_range()
> specifically states it cannot be used inside a spinlock.
>
> Fixes: 506b0a395f26 ("[netdrv] tg3: APE heartbeat changes")
> Signed-off-by: Jonathan Toppins <jtopp...@redhat.com>
> ---
>
> Notes:
> The thing I need reviewed from Broadcom is if the udelay should be 20
> instead of 10, due to any timing changes introduced by the offending
> patch.

Thanks.  10 us is correct.

As a future improvement, we might want to see if we can release the
spinlock and go back to usleep_range().  The wait time is potentially
up to 20 msec which is quite long.

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH net] tg3: prevent scheduling while atomic splat

[Michael Chan]

On Wed, Mar 14, 2018 at 9:36 AM, Jonathan Toppins  wrote:
> The problem was introduced in commit
> 506b0a395f26 ("[netdrv] tg3: APE heartbeat changes"). The bug occurs
> because tp->lock spinlock is held which is obtained in tg3_start
> by way of tg3_full_lock(), line 11571. The documentation for usleep_range()
> specifically states it cannot be used inside a spinlock.
>
> Fixes: 506b0a395f26 ("[netdrv] tg3: APE heartbeat changes")
> Signed-off-by: Jonathan Toppins 
> ---
>
> Notes:
> The thing I need reviewed from Broadcom is if the udelay should be 20
> instead of 10, due to any timing changes introduced by the offending
> patch.

Thanks.  10 us is correct.

As a future improvement, we might want to see if we can release the
spinlock and go back to usleep_range().  The wait time is potentially
up to 20 msec which is quite long.

Acked-by: Michael Chan 

Re: [PATCH] bnxt_en: don't update cpr->rx_bytes with uninitialized length len

[Michael Chan]

On Tue, Jan 16, 2018 at 2:22 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> Currently in the cases where cmp_type == CMP_TYPE_RX_L2_TPA_START_CMP or
> CMP_TYPE_RX_L2_TPA_END_CMP the exit path updates cpr->rx_bytes with an
> uninitialized length len.  Fix this by adding a new exit path that does
> not update the cpr stats with the bogus length len and remove the unused
> label next_rx_no_prod.
>
> Detected by CoverityScan, CID#1463807 ("Uninitialized scalar variable")
> Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt 
> moderation")
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Thanks.

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH] bnxt_en: don't update cpr->rx_bytes with uninitialized length len

[Michael Chan]

On Tue, Jan 16, 2018 at 2:22 AM, Colin King  wrote:
> From: Colin Ian King 
>
> Currently in the cases where cmp_type == CMP_TYPE_RX_L2_TPA_START_CMP or
> CMP_TYPE_RX_L2_TPA_END_CMP the exit path updates cpr->rx_bytes with an
> uninitialized length len.  Fix this by adding a new exit path that does
> not update the cpr stats with the bogus length len and remove the unused
> label next_rx_no_prod.
>
> Detected by CoverityScan, CID#1463807 ("Uninitialized scalar variable")
> Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt 
> moderation")
>
> Signed-off-by: Colin Ian King 

Thanks.

Acked-by: Michael Chan 

Re: [PATCH][next] bnxt_en: ensure len is ininitialized to zero

[Michael Chan]

On Fri, Jan 12, 2018 at 9:46 AM, Colin King  wrote:
> From: Colin Ian King 
>
> In the case where cmp_type == CMP_TYPE_RX_L2_TPA_START_CMP the
> exit return path is via label next_rx_no_prod and cpr->rx_bytes
> is being updated by an uninitialized value from len. Fix this by
> initializing len to zero.
>
> Detected by CoverityScan, CID#1463807 ("Uninitialized scalar variable")
>
> Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt 
> moderation")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index cf6ebf1e324b..5b5c4f266f1b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1482,7 +1482,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct 
> bnxt_napi *bnapi, u32 *raw_cons,
> u32 tmp_raw_cons = *raw_cons;
> u16 cfa_code, cons, prod, cp_cons = RING_CMP(tmp_raw_cons);
> struct bnxt_sw_rx_bd *rx_buf;
> -   unsigned int len;
> +   unsigned int len = 0;

It might be better to add a new label next_rx_no_prod_no_len and have
the TPA code paths jump there instead.

Andy, what do you think?

> u8 *data_ptr, agg_bufs, cmp_type;
> dma_addr_t dma_addr;
> struct sk_buff *skb;
> --
> 2.15.1
>

Re: [PATCH][next] bnxt_en: ensure len is ininitialized to zero

[Michael Chan]

On Fri, Jan 12, 2018 at 9:46 AM, Colin King  wrote:
> From: Colin Ian King 
>
> In the case where cmp_type == CMP_TYPE_RX_L2_TPA_START_CMP the
> exit return path is via label next_rx_no_prod and cpr->rx_bytes
> is being updated by an uninitialized value from len. Fix this by
> initializing len to zero.
>
> Detected by CoverityScan, CID#1463807 ("Uninitialized scalar variable")
>
> Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt 
> moderation")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index cf6ebf1e324b..5b5c4f266f1b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1482,7 +1482,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct 
> bnxt_napi *bnapi, u32 *raw_cons,
> u32 tmp_raw_cons = *raw_cons;
> u16 cfa_code, cons, prod, cp_cons = RING_CMP(tmp_raw_cons);
> struct bnxt_sw_rx_bd *rx_buf;
> -   unsigned int len;
> +   unsigned int len = 0;

It might be better to add a new label next_rx_no_prod_no_len and have
the TPA code paths jump there instead.

Andy, what do you think?

> u8 *data_ptr, agg_bufs, cmp_type;
> dma_addr_t dma_addr;
> struct sk_buff *skb;
> --
> 2.15.1
>

Re: [PATCH] bnxt_en: Fix sources of spurious netpoll warnings

[Michael Chan]

On Fri, Dec 8, 2017 at 9:05 AM, Calvin Owens <calvinow...@fb.com> wrote:
> After applying 2270bc5da3497945 ("bnxt_en: Fix netpoll handling") and
> 903649e718f80da2 ("bnxt_en: Improve -ENOMEM logic in NAPI poll loop."),
> we still see the following WARN fire:
>
>   [ cut here ]
>   WARNING: CPU: 0 PID: 1875170 at net/core/netpoll.c:165 
> netpoll_poll_dev+0x15a/0x160
>   bnxt_poll+0x0/0xd0 exceeded budget in poll
>   
>   Call Trace:
>[] dump_stack+0x4d/0x70
>[] __warn+0xd3/0xf0
>[] warn_slowpath_fmt+0x4f/0x60
>[] netpoll_poll_dev+0x15a/0x160
>[] netpoll_send_skb_on_dev+0x168/0x250
>[] netpoll_send_udp+0x2dc/0x440
>[] write_ext_msg+0x20e/0x250
>[] call_console_drivers.constprop.23+0xa5/0x110
>[] console_unlock+0x339/0x5b0
>[] vprintk_emit+0x2c8/0x450
>[] vprintk_default+0x1f/0x30
>[] printk+0x48/0x50
>[] edac_raw_mc_handle_error+0x563/0x5c0 [edac_core]
>[] edac_mc_handle_error+0x42b/0x6e0 [edac_core]
>[] sbridge_mce_output_error+0x410/0x10d0 [sb_edac]
>[] sbridge_check_error+0xac/0x130 [sb_edac]
>[] edac_mc_workq_function+0x3c/0x90 [edac_core]
>[] process_one_work+0x19b/0x480
>[] worker_thread+0x6a/0x520
>[] kthread+0xe4/0x100
>[] ret_from_fork+0x22/0x40
>
> This happens because we increment rx_pkts on -ENOMEM and -EIO, resulting
> in rx_pkts > 0. Fix this by only bumping rx_pkts if we were actually
> given a non-zero budget.
>
> Signed-off-by: Calvin Owens <calvinow...@fb.com>

Thanks.
Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH] bnxt_en: Fix sources of spurious netpoll warnings

[Michael Chan]

On Fri, Dec 8, 2017 at 9:05 AM, Calvin Owens  wrote:
> After applying 2270bc5da3497945 ("bnxt_en: Fix netpoll handling") and
> 903649e718f80da2 ("bnxt_en: Improve -ENOMEM logic in NAPI poll loop."),
> we still see the following WARN fire:
>
>   [ cut here ]
>   WARNING: CPU: 0 PID: 1875170 at net/core/netpoll.c:165 
> netpoll_poll_dev+0x15a/0x160
>   bnxt_poll+0x0/0xd0 exceeded budget in poll
>   
>   Call Trace:
>[] dump_stack+0x4d/0x70
>[] __warn+0xd3/0xf0
>[] warn_slowpath_fmt+0x4f/0x60
>[] netpoll_poll_dev+0x15a/0x160
>[] netpoll_send_skb_on_dev+0x168/0x250
>[] netpoll_send_udp+0x2dc/0x440
>[] write_ext_msg+0x20e/0x250
>[] call_console_drivers.constprop.23+0xa5/0x110
>[] console_unlock+0x339/0x5b0
>[] vprintk_emit+0x2c8/0x450
>[] vprintk_default+0x1f/0x30
>[] printk+0x48/0x50
>[] edac_raw_mc_handle_error+0x563/0x5c0 [edac_core]
>[] edac_mc_handle_error+0x42b/0x6e0 [edac_core]
>[] sbridge_mce_output_error+0x410/0x10d0 [sb_edac]
>[] sbridge_check_error+0xac/0x130 [sb_edac]
>[] edac_mc_workq_function+0x3c/0x90 [edac_core]
>[] process_one_work+0x19b/0x480
>[] worker_thread+0x6a/0x520
>[] kthread+0xe4/0x100
>[] ret_from_fork+0x22/0x40
>
> This happens because we increment rx_pkts on -ENOMEM and -EIO, resulting
> in rx_pkts > 0. Fix this by only bumping rx_pkts if we were actually
> given a non-zero budget.
>
> Signed-off-by: Calvin Owens 

Thanks.
Acked-by: Michael Chan 

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Thu, Dec 7, 2017 at 1:39 AM, Thomas Bogendoerfer
<tbogendoer...@suse.de> wrote:
> On Thu, 7 Dec 2017 01:24:43 -0800
> Michael Chan <michael.c...@broadcom.com> wrote:
>
>> On Thu, Dec 7, 2017 at 1:14 AM, Thomas Bogendoerfer
>> <tbogendoer...@suse.de> wrote:
>> > well, it will print the forced rate, if there is one configured and -1 
>> > otherwise,
>> > if the link is lost or will not come up because of a cable problem. I 
>> > don't see much
>> > value in that...
>> >
>> The main purpose is to tell the user that the speed he selected for a
>> port is no longer supported due to an incompatible speed configured on
>> the other port.  This is useful for the user so that he can either
>> take action to change the speed or do nothing as he sees fit.
>
> just out of curiosity what's meant my imcompatible speed on the other port ?
> Does the message show up continously like in the problem case I already know ?
>

On some dual-port cards, 10G and 25G are not compatible.  If one port
links up at 25G, for example, the other port will never link up if it
is set to 10G.  In this case the 10G port gets a message from the
firmware and the driver will print the "10G no longer supported"
message.  It's supposed to be a one time message.

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Thu, Dec 7, 2017 at 1:39 AM, Thomas Bogendoerfer
 wrote:
> On Thu, 7 Dec 2017 01:24:43 -0800
> Michael Chan  wrote:
>
>> On Thu, Dec 7, 2017 at 1:14 AM, Thomas Bogendoerfer
>>  wrote:
>> > well, it will print the forced rate, if there is one configured and -1 
>> > otherwise,
>> > if the link is lost or will not come up because of a cable problem. I 
>> > don't see much
>> > value in that...
>> >
>> The main purpose is to tell the user that the speed he selected for a
>> port is no longer supported due to an incompatible speed configured on
>> the other port.  This is useful for the user so that he can either
>> take action to change the speed or do nothing as he sees fit.
>
> just out of curiosity what's meant my imcompatible speed on the other port ?
> Does the message show up continously like in the problem case I already know ?
>

On some dual-port cards, 10G and 25G are not compatible.  If one port
links up at 25G, for example, the other port will never link up if it
is set to 10G.  In this case the 10G port gets a message from the
firmware and the driver will print the "10G no longer supported"
message.  It's supposed to be a one time message.

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Thu, Dec 7, 2017 at 1:14 AM, Thomas Bogendoerfer
<tbogendoer...@suse.de> wrote:
> On Wed, 6 Dec 2017 11:27:31 -0800
> Michael Chan <michael.c...@broadcom.com> wrote:
>
>> On Tue, Dec 5, 2017 at 4:33 AM, Thomas Bogendoerfer
>> <tbogendoer...@suse.de> wrote:
>> > bnxt driver spams logfiles with
>> >
>> > [  541.003065] bnxt_en :5d:00.1 eth5: Link speed -1 no longer supported
>> >
>> > if a direct attached cable (DAC) is plugged into the bnxt card and is
>> > unplugged on the other side. This patch removes the code printing this
>> > message, since it doesn't provide any useful information.
>> >
>> > Signed-off-by: Thomas Bogendoerfer <tbogendoer...@suse.de>
>> > ---
>> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 -
>> >  1 file changed, 9 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
>> > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > index 8c1dd60eab6f..8a2319ed79dc 100644
>> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > @@ -1701,18 +1701,9 @@ static int bnxt_async_event_process(struct bnxt *bp,
>> > /* TODO CHIMP_FW: Define event id's for link change, error etc */
>> > switch (event_id) {
>> > case ASYNC_EVENT_CMPL_EVENT_ID_LINK_SPEED_CFG_CHANGE: {
>> > -   u32 data1 = le32_to_cpu(cmpl->event_data1);
>> > -   struct bnxt_link_info *link_info = >link_info;
>> > -
>> > if (BNXT_VF(bp))
>> > goto async_event_process_exit;
>> > -   if (data1 & 0x2) {
>> > -   u16 fw_speed = link_info->force_link_speed;
>> > -   u32 speed = bnxt_fw_to_ethtool_speed(fw_speed);
>> >
>> > -   netdev_warn(bp->dev, "Link speed %d no longer 
>> > supported\n",
>> > -   speed);
>> > -   }
>>
>> This is supposed to provide useful information to the user under some
>> conditions.
>
> well, it will print the forced rate, if there is one configured and -1 
> otherwise,
> if the link is lost or will not come up because of a cable problem. I don't 
> see much
> value in that...
>
The main purpose is to tell the user that the speed he selected for a
port is no longer supported due to an incompatible speed configured on
the other port.  This is useful for the user so that he can either
take action to change the speed or do nothing as he sees fit.

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Thu, Dec 7, 2017 at 1:14 AM, Thomas Bogendoerfer
 wrote:
> On Wed, 6 Dec 2017 11:27:31 -0800
> Michael Chan  wrote:
>
>> On Tue, Dec 5, 2017 at 4:33 AM, Thomas Bogendoerfer
>>  wrote:
>> > bnxt driver spams logfiles with
>> >
>> > [  541.003065] bnxt_en :5d:00.1 eth5: Link speed -1 no longer supported
>> >
>> > if a direct attached cable (DAC) is plugged into the bnxt card and is
>> > unplugged on the other side. This patch removes the code printing this
>> > message, since it doesn't provide any useful information.
>> >
>> > Signed-off-by: Thomas Bogendoerfer 
>> > ---
>> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 -
>> >  1 file changed, 9 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
>> > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > index 8c1dd60eab6f..8a2319ed79dc 100644
>> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > @@ -1701,18 +1701,9 @@ static int bnxt_async_event_process(struct bnxt *bp,
>> > /* TODO CHIMP_FW: Define event id's for link change, error etc */
>> > switch (event_id) {
>> > case ASYNC_EVENT_CMPL_EVENT_ID_LINK_SPEED_CFG_CHANGE: {
>> > -   u32 data1 = le32_to_cpu(cmpl->event_data1);
>> > -   struct bnxt_link_info *link_info = >link_info;
>> > -
>> > if (BNXT_VF(bp))
>> > goto async_event_process_exit;
>> > -   if (data1 & 0x2) {
>> > -   u16 fw_speed = link_info->force_link_speed;
>> > -   u32 speed = bnxt_fw_to_ethtool_speed(fw_speed);
>> >
>> > -   netdev_warn(bp->dev, "Link speed %d no longer 
>> > supported\n",
>> > -   speed);
>> > -   }
>>
>> This is supposed to provide useful information to the user under some
>> conditions.
>
> well, it will print the forced rate, if there is one configured and -1 
> otherwise,
> if the link is lost or will not come up because of a cable problem. I don't 
> see much
> value in that...
>
The main purpose is to tell the user that the speed he selected for a
port is no longer supported due to an incompatible speed configured on
the other port.  This is useful for the user so that he can either
take action to change the speed or do nothing as he sees fit.

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Tue, Dec 5, 2017 at 4:33 AM, Thomas Bogendoerfer
 wrote:
> bnxt driver spams logfiles with
>
> [  541.003065] bnxt_en :5d:00.1 eth5: Link speed -1 no longer supported
>
> if a direct attached cable (DAC) is plugged into the bnxt card and is
> unplugged on the other side. This patch removes the code printing this
> message, since it doesn't provide any useful information.
>
> Signed-off-by: Thomas Bogendoerfer 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 8c1dd60eab6f..8a2319ed79dc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1701,18 +1701,9 @@ static int bnxt_async_event_process(struct bnxt *bp,
> /* TODO CHIMP_FW: Define event id's for link change, error etc */
> switch (event_id) {
> case ASYNC_EVENT_CMPL_EVENT_ID_LINK_SPEED_CFG_CHANGE: {
> -   u32 data1 = le32_to_cpu(cmpl->event_data1);
> -   struct bnxt_link_info *link_info = >link_info;
> -
> if (BNXT_VF(bp))
> goto async_event_process_exit;
> -   if (data1 & 0x2) {
> -   u16 fw_speed = link_info->force_link_speed;
> -   u32 speed = bnxt_fw_to_ethtool_speed(fw_speed);
>
> -   netdev_warn(bp->dev, "Link speed %d no longer 
> supported\n",
> -   speed);
> -   }

This is supposed to provide useful information to the user under some
conditions.  In your particular situation, it is not useful since the
speed is -1.  Let me try to modify this code a bit to reduce the spam.
I will post a revised patch in the next 2 hours.

Thanks.

> set_bit(BNXT_LINK_SPEED_CHNG_SP_EVENT, >sp_event);
> /* fall thru */
> }
> --
> 2.12.3
>

Re: [PATCH net-next] bnxt: Don't print message, if DAC isn't connected on both ends

[Michael Chan]

On Tue, Dec 5, 2017 at 4:33 AM, Thomas Bogendoerfer
 wrote:
> bnxt driver spams logfiles with
>
> [  541.003065] bnxt_en :5d:00.1 eth5: Link speed -1 no longer supported
>
> if a direct attached cable (DAC) is plugged into the bnxt card and is
> unplugged on the other side. This patch removes the code printing this
> message, since it doesn't provide any useful information.
>
> Signed-off-by: Thomas Bogendoerfer 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 8c1dd60eab6f..8a2319ed79dc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1701,18 +1701,9 @@ static int bnxt_async_event_process(struct bnxt *bp,
> /* TODO CHIMP_FW: Define event id's for link change, error etc */
> switch (event_id) {
> case ASYNC_EVENT_CMPL_EVENT_ID_LINK_SPEED_CFG_CHANGE: {
> -   u32 data1 = le32_to_cpu(cmpl->event_data1);
> -   struct bnxt_link_info *link_info = >link_info;
> -
> if (BNXT_VF(bp))
> goto async_event_process_exit;
> -   if (data1 & 0x2) {
> -   u16 fw_speed = link_info->force_link_speed;
> -   u32 speed = bnxt_fw_to_ethtool_speed(fw_speed);
>
> -   netdev_warn(bp->dev, "Link speed %d no longer 
> supported\n",
> -   speed);
> -   }

This is supposed to provide useful information to the user under some
conditions.  In your particular situation, it is not useful since the
speed is -1.  Let me try to modify this code a bit to reduce the spam.
I will post a revised patch in the next 2 hours.

Thanks.

> set_bit(BNXT_LINK_SPEED_CHNG_SP_EVENT, >sp_event);
> /* fall thru */
> }
> --
> 2.12.3
>

Re: [PATCH 2/2] bnxt_en: Add ETH_RESET_AP support

[Michael Chan]

On Thu, Nov 30, 2017 at 11:36 AM, Scott Branden
<scott.bran...@broadcom.com> wrote:
> Add ETH_RESET_AP support handling to reset the internal
> Application Processor(s) of the SmartNIC card.
>
> Signed-off-by: Scott Branden <scott.bran...@broadcom.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH 2/2] bnxt_en: Add ETH_RESET_AP support

[Michael Chan]

On Thu, Nov 30, 2017 at 11:36 AM, Scott Branden
 wrote:
> Add ETH_RESET_AP support handling to reset the internal
> Application Processor(s) of the SmartNIC card.
>
> Signed-off-by: Scott Branden 

Acked-by: Michael Chan 

Re: [PATCH] bnxt_en: Fix an error handling path in 'bnxt_get_module_eeprom()'

[Michael Chan]

On Tue, Nov 28, 2017 at 7:56 AM, David Miller  wrote:
> From: Christophe JAILLET 
> Date: Tue, 21 Nov 2017 20:46:49 +0100
>
>> Error code returned by 'bnxt_read_sfp_module_eeprom_info()' is handled a
>> few lines above when reading the A0 portion of the EEPROM.
>> The same should be done when reading the A2 portion of the EEPROM.
>>
>> In order to correctly propagate an error, update 'rc' in this 2nd call as
>> well, otherwise 0 (success) is returned.
>>
>> Signed-off-by: Christophe JAILLET 
>
> Patch applied, thanks Chrisophe.
>
> I cannot see any legitimate reason to ignore errors returned here, as
> an error would mean a partial read back of the data to the caller.

Sorry I was on vacation and missed this earlier.  The reason we don't
check for errors in the 2nd part is that the 2nd page may not be
present on some eeproms.

But I think the patch is fine because we return the proper length in
.get_module_info().  So we will now return error only if the user
specifies an invalid length, which is fine.  Thanks.

Re: [PATCH] bnxt_en: Fix an error handling path in 'bnxt_get_module_eeprom()'

[Michael Chan]

On Tue, Nov 28, 2017 at 7:56 AM, David Miller  wrote:
> From: Christophe JAILLET 
> Date: Tue, 21 Nov 2017 20:46:49 +0100
>
>> Error code returned by 'bnxt_read_sfp_module_eeprom_info()' is handled a
>> few lines above when reading the A0 portion of the EEPROM.
>> The same should be done when reading the A2 portion of the EEPROM.
>>
>> In order to correctly propagate an error, update 'rc' in this 2nd call as
>> well, otherwise 0 (success) is returned.
>>
>> Signed-off-by: Christophe JAILLET 
>
> Patch applied, thanks Chrisophe.
>
> I cannot see any legitimate reason to ignore errors returned here, as
> an error would mean a partial read back of the data to the caller.

Sorry I was on vacation and missed this earlier.  The reason we don't
check for errors in the 2nd part is that the 2nd page may not be
present on some eeproms.

But I think the patch is fine because we return the proper length in
.get_module_info().  So we will now return error only if the user
specifies an invalid length, which is fine.  Thanks.

Re: [PATCH net-next 2/2] bnxt_en: tc: only the function prototypes need to be wrapped in #ifdef

[Michael Chan]

On Fri, Oct 6, 2017 at 12:48 PM, Jonathan Toppins  wrote:
> There is no reason to wrap the data structures inside the ifdef.

What's so bad about wrapping unused data structures inside #ifdef?
These structures are only used if CONFIG_BNXT_FLOWER_OFFLOAD is
defined.

Re: [PATCH net-next 2/2] bnxt_en: tc: only the function prototypes need to be wrapped in #ifdef

[Michael Chan]

On Fri, Oct 6, 2017 at 12:48 PM, Jonathan Toppins  wrote:
> There is no reason to wrap the data structures inside the ifdef.

What's so bad about wrapping unused data structures inside #ifdef?
These structures are only used if CONFIG_BNXT_FLOWER_OFFLOAD is
defined.

Re: [PATCH net-next 1/2] bnxt_en: don't consider building bnxt_tc.o if option not enabled

[Michael Chan]

On Fri, Oct 6, 2017 at 12:48 PM, Jonathan Toppins <jtopp...@redhat.com> wrote:
> Instead of zeroing out bnxt_tc.c with a #ifdef foo, instead don't compile
> the file when the option is not enabled. Now make and the preprocessor do
> not have to waste time compiling a no-op.
>
> Signed-off-by: Jonathan Toppins <jtopp...@redhat.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH net-next 1/2] bnxt_en: don't consider building bnxt_tc.o if option not enabled

[Michael Chan]

On Fri, Oct 6, 2017 at 12:48 PM, Jonathan Toppins  wrote:
> Instead of zeroing out bnxt_tc.c with a #ifdef foo, instead don't compile
> the file when the option is not enabled. Now make and the preprocessor do
> not have to waste time compiling a no-op.
>
> Signed-off-by: Jonathan Toppins 

Acked-by: Michael Chan 

Re: [PATCH net-next v2] tg3: Be drop monitor friendly

[Michael Chan]

On Thu, Aug 24, 2017 at 5:47 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> tg3_tx() does the normal packet TX completion,
> tigon3_dma_hwbug_workaround() and tg3_tso_bug() both need to allocate a
> new SKB that is suitable to workaround HW bugs, and finally
> tg3_free_rings() is doing ring cleanup. Use dev_consume_skb_any() for
> these 3 locations to be SKB drop monitor friendly.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

> ---
> Changes in v2:
>
> - also included tg3_tso_bug() as indicated by Michael
>

Re: [PATCH net-next v2] tg3: Be drop monitor friendly

[Michael Chan]

On Thu, Aug 24, 2017 at 5:47 PM, Florian Fainelli  wrote:
> tg3_tx() does the normal packet TX completion,
> tigon3_dma_hwbug_workaround() and tg3_tso_bug() both need to allocate a
> new SKB that is suitable to workaround HW bugs, and finally
> tg3_free_rings() is doing ring cleanup. Use dev_consume_skb_any() for
> these 3 locations to be SKB drop monitor friendly.
>
> Signed-off-by: Florian Fainelli 

Acked-by: Michael Chan 

> ---
> Changes in v2:
>
> - also included tg3_tso_bug() as indicated by Michael
>

Re: [PATCH net 1/7] b44: Initialize 64-bit stats seqcount

[Michael Chan]

On Tue, Aug 1, 2017 at 12:11 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> On 32-bit hosts and with CONFIG_DEBUG_LOCK_ALLOC we should be seeing a
> lockdep splat indicating this seqcount is not correctly initialized, fix
> that.
>
> Fixes: eeda8585522b ("b44: add 64 bit stats")
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

Thanks.

Re: [PATCH net 1/7] b44: Initialize 64-bit stats seqcount

[Michael Chan]

On Tue, Aug 1, 2017 at 12:11 PM, Florian Fainelli  wrote:
> On 32-bit hosts and with CONFIG_DEBUG_LOCK_ALLOC we should be seeing a
> lockdep splat indicating this seqcount is not correctly initialized, fix
> that.
>
> Fixes: eeda8585522b ("b44: add 64 bit stats")
> Signed-off-by: Florian Fainelli 

Acked-by: Michael Chan 

Thanks.

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Michael Chan]

On Tue, Jul 25, 2017 at 8:29 AM, Arnd Bergmann  wrote:
> The sriov_lock is used to serialize the sriov code with the vfr code.
> However, when SRIOV is disabled, the lock is not there at all, leading
> to a build error:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: In function 
> 'bnxt_dl_eswitch_mode_set':
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:410:16: error: 'struct bnxt' 
> has no member named 'sriov_lock'
>
> We can either provide the mutex in this configuration, too, or
> disable both SRIOV and VFR together. This implements the first
> approach, since it seems like a reasonable configuration for
> guest kernels to have, and the extra lock will be harmless when
> there is no contention.
>
> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> Signed-off-by: Arnd Bergmann 

Sathya already sent 3 patches to fix some of these issues.  But I need
to rework one of his patch and resend.

Thanks.

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Michael Chan]

On Tue, Jul 25, 2017 at 8:29 AM, Arnd Bergmann  wrote:
> The sriov_lock is used to serialize the sriov code with the vfr code.
> However, when SRIOV is disabled, the lock is not there at all, leading
> to a build error:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: In function 
> 'bnxt_dl_eswitch_mode_set':
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:410:16: error: 'struct bnxt' 
> has no member named 'sriov_lock'
>
> We can either provide the mutex in this configuration, too, or
> disable both SRIOV and VFR together. This implements the first
> approach, since it seems like a reasonable configuration for
> guest kernels to have, and the extra lock will be harmless when
> there is no contention.
>
> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> Signed-off-by: Arnd Bergmann 

Sathya already sent 3 patches to fix some of these issues.  But I need
to rework one of his patch and resend.

Thanks.

Re: [PATCH v2 03/22] net: broadcom: stop using rtc deprecated functions

[Michael Chan]

On Wed, Jul 12, 2017 at 1:04 AM, Benjamin Gaignard
<benjamin.gaign...@linaro.org> wrote:
> rtc_time_to_tm() and rtc_tm_to_time() are deprecated because they
> rely on 32bits variables and that will make rtc break in y2038/2016.
> Stop using those two functions to safer 64bits ones.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@linaro.org>
> CC: Michael Chan <michael.c...@broadcom.com>
> CC: net...@vger.kernel.org
> CC: linux-kernel@vger.kernel.org

Acked-by: Michael Chan <michael.c...@broadcom.com>

Thanks.

Re: [PATCH v2 03/22] net: broadcom: stop using rtc deprecated functions

[Michael Chan]

On Wed, Jul 12, 2017 at 1:04 AM, Benjamin Gaignard
 wrote:
> rtc_time_to_tm() and rtc_tm_to_time() are deprecated because they
> rely on 32bits variables and that will make rtc break in y2038/2016.
> Stop using those two functions to safer 64bits ones.
>
> Signed-off-by: Benjamin Gaignard 
> CC: Michael Chan 
> CC: net...@vger.kernel.org
> CC: linux-kernel@vger.kernel.org

Acked-by: Michael Chan 

Thanks.

Re: [PATCH] net: ethernet: broadcom: bnxt: remove dead code

[Michael Chan]

On Mon, May 15, 2017 at 3:28 PM, Gustavo A. R. Silva
 wrote:
> Local variable _sh_ is assigned to a constant value and it is never updated
> again. Remove this variable and the dead code it guards.
>
> Addresses-Coverity-ID: 1350916
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 1f1e54b..018674b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -7380,12 +7380,10 @@ static int bnxt_get_dflt_rings(struct bnxt *bp, int 
> *max_rx, int *max_tx,
>  static int bnxt_set_dflt_rings(struct bnxt *bp)
>  {
> int dflt_rings, max_rx_rings, max_tx_rings, rc;
> -   bool sh = true;

The point of this logic is that we can easily change the default to
shared rings or separate rings.  I think what I'll do is change it so
that the parameter sh is passed in and let the caller decide.  Thanks
for pointing this out.

>
> -   if (sh)
> -   bp->flags |= BNXT_FLAG_SHARED_RINGS;
> +   bp->flags |= BNXT_FLAG_SHARED_RINGS;
> dflt_rings = netif_get_num_default_rss_queues();
> -   rc = bnxt_get_dflt_rings(bp, _rx_rings, _tx_rings, sh);
> +   rc = bnxt_get_dflt_rings(bp, _rx_rings, _tx_rings, true);
> if (rc)
> return rc;
> bp->rx_nr_rings = min_t(int, dflt_rings, max_rx_rings);
> @@ -7396,8 +7394,7 @@ static int bnxt_set_dflt_rings(struct bnxt *bp)
> netdev_warn(bp->dev, "Unable to reserve tx rings\n");
>
> bp->tx_nr_rings = bp->tx_nr_rings_per_tc;
> -   bp->cp_nr_rings = sh ? max_t(int, bp->tx_nr_rings, bp->rx_nr_rings) :
> -  bp->tx_nr_rings + bp->rx_nr_rings;
> +   bp->cp_nr_rings = max_t(int, bp->tx_nr_rings, bp->rx_nr_rings);
> bp->num_stat_ctxs = bp->cp_nr_rings;
> if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
> bp->rx_nr_rings++;
> --
> 2.5.0
>

Re: [PATCH] net: ethernet: broadcom: bnxt: remove dead code

[Michael Chan]

On Mon, May 15, 2017 at 3:28 PM, Gustavo A. R. Silva
 wrote:
> Local variable _sh_ is assigned to a constant value and it is never updated
> again. Remove this variable and the dead code it guards.
>
> Addresses-Coverity-ID: 1350916
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 1f1e54b..018674b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -7380,12 +7380,10 @@ static int bnxt_get_dflt_rings(struct bnxt *bp, int 
> *max_rx, int *max_tx,
>  static int bnxt_set_dflt_rings(struct bnxt *bp)
>  {
> int dflt_rings, max_rx_rings, max_tx_rings, rc;
> -   bool sh = true;

The point of this logic is that we can easily change the default to
shared rings or separate rings.  I think what I'll do is change it so
that the parameter sh is passed in and let the caller decide.  Thanks
for pointing this out.

>
> -   if (sh)
> -   bp->flags |= BNXT_FLAG_SHARED_RINGS;
> +   bp->flags |= BNXT_FLAG_SHARED_RINGS;
> dflt_rings = netif_get_num_default_rss_queues();
> -   rc = bnxt_get_dflt_rings(bp, _rx_rings, _tx_rings, sh);
> +   rc = bnxt_get_dflt_rings(bp, _rx_rings, _tx_rings, true);
> if (rc)
> return rc;
> bp->rx_nr_rings = min_t(int, dflt_rings, max_rx_rings);
> @@ -7396,8 +7394,7 @@ static int bnxt_set_dflt_rings(struct bnxt *bp)
> netdev_warn(bp->dev, "Unable to reserve tx rings\n");
>
> bp->tx_nr_rings = bp->tx_nr_rings_per_tc;
> -   bp->cp_nr_rings = sh ? max_t(int, bp->tx_nr_rings, bp->rx_nr_rings) :
> -  bp->tx_nr_rings + bp->rx_nr_rings;
> +   bp->cp_nr_rings = max_t(int, bp->tx_nr_rings, bp->rx_nr_rings);
> bp->num_stat_ctxs = bp->cp_nr_rings;
> if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
> bp->rx_nr_rings++;
> --
> 2.5.0
>

Re: [PATCH 1/1] netdev: broadcom: propagate error code

[Michael Chan]

On Sat, Dec 3, 2016 at 1:56 AM, Pan Bian <bianpan2...@163.com> wrote:
> Function bnxt_hwrm_stat_ctx_alloc() always returns 0, even if the call
> to _hwrm_send_message() fails. It may be better to propagate the errors
> to the caller of bnxt_hwrm_stat_ctx_alloc().
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188661
>
> Signed-off-by: Pan Bian <bianpan2...@163.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH 1/1] netdev: broadcom: propagate error code

[Michael Chan]

On Sat, Dec 3, 2016 at 1:56 AM, Pan Bian  wrote:
> Function bnxt_hwrm_stat_ctx_alloc() always returns 0, even if the call
> to _hwrm_send_message() fails. It may be better to propagate the errors
> to the caller of bnxt_hwrm_stat_ctx_alloc().
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188661
>
> Signed-off-by: Pan Bian 

Acked-by: Michael Chan 

Re: [PATCH v2 2/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Michael Chan]

On Sat, Nov 12, 2016 at 9:01 PM, Baoquan He <b...@redhat.com> wrote:
> In-flight DMA from 1st kernel could continue going in kdump kernel.
> New io-page table has been created before bnx2 does reset at open stage.
> We have to wait for the in-flight DMA to complete to avoid it look up
> into the newly created io-page table at probe stage.
>
> Suggested-by: Michael Chan <michael.c...@broadcom.com>
> Signed-off-by: Baoquan He <b...@redhat.com>

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH v2 2/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Michael Chan]

On Sat, Nov 12, 2016 at 9:01 PM, Baoquan He  wrote:
> In-flight DMA from 1st kernel could continue going in kdump kernel.
> New io-page table has been created before bnx2 does reset at open stage.
> We have to wait for the in-flight DMA to complete to avoid it look up
> into the newly created io-page table at probe stage.
>
> Suggested-by: Michael Chan 
> Signed-off-by: Baoquan He 

Acked-by: Michael Chan 

Re: [PATCH 0/2] bnx2: Hard reset bnx2 chip at probe stage

[Michael Chan]

On Fri, Nov 11, 2016 at 6:02 AM, Baoquan He  wrote:
> On 11/11/16 at 09:46pm, Baoquan He wrote:
>> Hi bnx2 experts,
>>
>> In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
>> firmware requesting code was moved from open stage to probe stage.
>> The reason is in kdump kernel hardware iommu need device be reset in
>> driver probe stage, otherwise those in-flight DMA from 1st kernel
>> will continue going and look up into the newly created io-page tables.
>> So we need reset device to stop in-flight DMA as early as possibe.
>>
>> But with commit 3e1be7a merged, people reported their bnx2 driver init
>> failed because of failed firmware loading. After discussion, it's found
>> that they built bnx2 driver into kernel, and that makes probe function
>> bnx2_init_one be called in do_initcalls(). But at this time the initramfs
>> has not been uncompressed yet and mounted, kernel can't detect firmware.
>>
>> So there's only one way to cover both. Try to hard reset the bnx2 device
>> at probe stage, without involving firmware issues. I tried to add function
>> bnx2_hard_reset_chip() to do this and it's only called in kdump kernel.
>> The thing is I am not quite familiar with bnx2 chip spec, just abstract
>> code from bnx2_reset_chip, the testing result is good.
>
> Here I changed to send BNX2_MISC_COMMAND_HD_RESET in BNX2_CHIP_5709
> case.
>

>From my old 5709 Documentation:

Bit 6 HD_RESET:  Writing this bit as 1 will cause the chip to do a
hard reset like bit 5 except the sticky bits in the PCI function are
not reset.

Bit 5 POR_RESET: Writing this bit as 1 will cause the chip to do an
internal reset exactly like a power-up reset.  There is no protection
for this request and it may cause any current PCI cycle to lock up.
This reset is intended for use under manufacturing conditions only.

So it sounds like doing HD_RESET can potentially cause a PCI bus lock up.

Why not just disable DMA gracefully as done at the beginning of
bnx2_reset_chip()?

Re: [PATCH 0/2] bnx2: Hard reset bnx2 chip at probe stage

[Michael Chan]

On Fri, Nov 11, 2016 at 6:02 AM, Baoquan He  wrote:
> On 11/11/16 at 09:46pm, Baoquan He wrote:
>> Hi bnx2 experts,
>>
>> In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
>> firmware requesting code was moved from open stage to probe stage.
>> The reason is in kdump kernel hardware iommu need device be reset in
>> driver probe stage, otherwise those in-flight DMA from 1st kernel
>> will continue going and look up into the newly created io-page tables.
>> So we need reset device to stop in-flight DMA as early as possibe.
>>
>> But with commit 3e1be7a merged, people reported their bnx2 driver init
>> failed because of failed firmware loading. After discussion, it's found
>> that they built bnx2 driver into kernel, and that makes probe function
>> bnx2_init_one be called in do_initcalls(). But at this time the initramfs
>> has not been uncompressed yet and mounted, kernel can't detect firmware.
>>
>> So there's only one way to cover both. Try to hard reset the bnx2 device
>> at probe stage, without involving firmware issues. I tried to add function
>> bnx2_hard_reset_chip() to do this and it's only called in kdump kernel.
>> The thing is I am not quite familiar with bnx2 chip spec, just abstract
>> code from bnx2_reset_chip, the testing result is good.
>
> Here I changed to send BNX2_MISC_COMMAND_HD_RESET in BNX2_CHIP_5709
> case.
>

>From my old 5709 Documentation:

Bit 6 HD_RESET:  Writing this bit as 1 will cause the chip to do a
hard reset like bit 5 except the sticky bits in the PCI function are
not reset.

Bit 5 POR_RESET: Writing this bit as 1 will cause the chip to do an
internal reset exactly like a power-up reset.  There is no protection
for this request and it may cause any current PCI cycle to lock up.
This reset is intended for use under manufacturing conditions only.

So it sounds like doing HD_RESET can potentially cause a PCI bus lock up.

Why not just disable DMA gracefully as done at the beginning of
bnx2_reset_chip()?

Re: [PATCH] net: tg3: use new api ethtool_{get|set}_link_ksettings

[Michael Chan]

On Sun, Sep 25, 2016 at 2:31 PM, Philippe Reynes <trem...@gmail.com> wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
>
> Signed-off-by: Philippe Reynes <trem...@gmail.com>

Looks good to me.  Thanks.
Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH] net: tg3: use new api ethtool_{get|set}_link_ksettings

[Michael Chan]

On Sun, Sep 25, 2016 at 2:31 PM, Philippe Reynes  wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
>
> Signed-off-by: Philippe Reynes 

Looks good to me.  Thanks.
Acked-by: Michael Chan 

Re: [PATCH net] bnxt_en: Remove locking around txr->dev_state

[Michael Chan]

On Mon, Jul 18, 2016 at 1:02 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> txr->dev_state was not consistently manipulated with the acquisition of
> the per-queue lock, after further inspection the lock does not seem
> necessary, either the value is read as BNXT_DEV_STATE_CLOSING or 0.
>
> Reported-by: coverity (CID 1339583)
> Fixes: c0c050c58d840 ("bnxt_en: New Broadcom ethernet driver.")
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>

Thanks Florian.

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH net] bnxt_en: Remove locking around txr->dev_state

[Michael Chan]

On Mon, Jul 18, 2016 at 1:02 PM, Florian Fainelli  wrote:
> txr->dev_state was not consistently manipulated with the acquisition of
> the per-queue lock, after further inspection the lock does not seem
> necessary, either the value is read as BNXT_DEV_STATE_CLOSING or 0.
>
> Reported-by: coverity (CID 1339583)
> Fixes: c0c050c58d840 ("bnxt_en: New Broadcom ethernet driver.")
> Signed-off-by: Florian Fainelli 

Thanks Florian.

Acked-by: Michael Chan 

Re: [PATCH net] bnxt_en: Fix potential race condition in bnxt_tx_enable()

[Michael Chan]

On Fri, Jul 15, 2016 at 11:20 PM, David Miller  wrote:
> From: Florian Fainelli 
> Date: Fri, 15 Jul 2016 16:42:01 -0700
>
>> @@ -4599,7 +4599,9 @@ static void bnxt_tx_enable(struct bnxt *bp)
>>   for (i = 0; i < bp->tx_nr_rings; i++) {
>>   txr = >tx_ring[i];
>>   txq = netdev_get_tx_queue(bp->dev, i);
>> + __netif_tx_lock(txq, smp_processor_id());
>>   txr->dev_state = 0;
>> + __netif_tx_unlock(txq);
>
> You're going to have to explain how this could possibly cause a
> problem, because I'm pretty sure it can't.
>
> Either the reader sees 0, or non-zero, in this value.
>
> And adding locking around this assignment does not change that at all.

Florian, I agree with David.  The lock is not needed.  The lock in
bnxt_tx_disable() is also unnecessary and should be removed.  Thanks.

Re: [PATCH net] bnxt_en: Fix potential race condition in bnxt_tx_enable()

[Michael Chan]

On Fri, Jul 15, 2016 at 11:20 PM, David Miller  wrote:
> From: Florian Fainelli 
> Date: Fri, 15 Jul 2016 16:42:01 -0700
>
>> @@ -4599,7 +4599,9 @@ static void bnxt_tx_enable(struct bnxt *bp)
>>   for (i = 0; i < bp->tx_nr_rings; i++) {
>>   txr = >tx_ring[i];
>>   txq = netdev_get_tx_queue(bp->dev, i);
>> + __netif_tx_lock(txq, smp_processor_id());
>>   txr->dev_state = 0;
>> + __netif_tx_unlock(txq);
>
> You're going to have to explain how this could possibly cause a
> problem, because I'm pretty sure it can't.
>
> Either the reader sees 0, or non-zero, in this value.
>
> And adding locking around this assignment does not change that at all.

Florian, I agree with David.  The lock is not needed.  The lock in
bnxt_tx_disable() is also unnecessary and should be removed.  Thanks.

Re: [PATCH] bnxt_en: initialize rc to zero to avoid returning garbage

[Michael Chan]

On Fri, Jul 8, 2016 at 8:42 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> rc is not initialized so it can contain garbage if it is not
> set by the call to bnxt_read_sfp_module_eeprom_info. Ensure
> garbage is not returned by initializing rc to 0.
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Thanks.

Acked-by: Michael Chan <michael.c...@broadcom.com>

Re: [PATCH] bnxt_en: initialize rc to zero to avoid returning garbage

[Michael Chan]

On Fri, Jul 8, 2016 at 8:42 AM, Colin King  wrote:
> From: Colin Ian King 
>
> rc is not initialized so it can contain garbage if it is not
> set by the call to bnxt_read_sfp_module_eeprom_info. Ensure
> garbage is not returned by initializing rc to 0.
>
> Signed-off-by: Colin Ian King 

Thanks.

Acked-by: Michael Chan 

Re: [PATCH] bnxt_en: add VXLAN dependency

[Michael Chan]

On Wed, 2015-11-04 at 16:00 +0100, Arnd Bergmann wrote: 
> VXLAN may be a loadable module, and this driver cannot be built-in
> in that case, or we get a link error:
> 
> drivers/built-in.o: In function `__bnxt_open_nic':
> drivers/net/ethernet/broadcom/bnxt/bnxt.c:4581: undefined reference to 
> `vxlan_get_rx_port'
> 
> This adds a Kconfig dependency that ensures that either VXLAN is
> disabled (which the driver handles correctly), or we depend on
> VXLAN itself and disallow built-in compilation when VXLAN is
> a module.
> 
> Signed-off-by: Arnd Bergmann 

Thanks.
Acked-by: Michael Chan 

> Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> 
> diff --git a/drivers/net/ethernet/broadcom/Kconfig 
> b/drivers/net/ethernet/broadcom/Kconfig
> index 67a7d520d9f5..8550df189ceb 100644
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig
> @@ -173,6 +173,7 @@ config SYSTEMPORT
>  config BNXT
>   tristate "Broadcom NetXtreme-C/E support"
>   depends on PCI
> + depends on VXLAN || VXLAN=n
>   select FW_LOADER
>   select LIBCRC32C
>   ---help---
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bnxt_en: add VXLAN dependency

[Michael Chan]

On Wed, 2015-11-04 at 16:00 +0100, Arnd Bergmann wrote: 
> VXLAN may be a loadable module, and this driver cannot be built-in
> in that case, or we get a link error:
> 
> drivers/built-in.o: In function `__bnxt_open_nic':
> drivers/net/ethernet/broadcom/bnxt/bnxt.c:4581: undefined reference to 
> `vxlan_get_rx_port'
> 
> This adds a Kconfig dependency that ensures that either VXLAN is
> disabled (which the driver handles correctly), or we depend on
> VXLAN itself and disallow built-in compilation when VXLAN is
> a module.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Thanks.
Acked-by: Michael Chan <mc...@broadcom.com>

> Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> 
> diff --git a/drivers/net/ethernet/broadcom/Kconfig 
> b/drivers/net/ethernet/broadcom/Kconfig
> index 67a7d520d9f5..8550df189ceb 100644
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig
> @@ -173,6 +173,7 @@ config SYSTEMPORT
>  config BNXT
>   tristate "Broadcom NetXtreme-C/E support"
>   depends on PCI
> + depends on VXLAN || VXLAN=n
>   select FW_LOADER
>   select LIBCRC32C
>   ---help---
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tg3:Add error handling to the function tg3_test_loopback

[Michael Chan]

On Thu, 2015-07-16 at 12:18 -0700, Michael Chan wrote: 
> On Thu, 2015-07-16 at 14:51 -0400, Nicholas Krause wrote: 
> > This adds proper error handling for if the calls to the function
> > tg3_phy_lpbk_set fail by returning -EIO by assigning the return
> > value to the variable err and if it equals anything other then
> > zero jumps to the goto label done as no other work can be handled
> > internally in the function tg3_test_loopback.
> > 
> > Signed-off-by: Nicholas Krause 
> 
> Acked-by: Michael Chan 

Your indentation doesn't look right.  Other than that, it is ok.

> 
> > ---
> >  drivers/net/ethernet/broadcom/tg3.c | 12 
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> > b/drivers/net/ethernet/broadcom/tg3.c
> > index 73c934c..8584cb7 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -13625,8 +13625,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
> > *data, bool do_extlpbk)
> > !tg3_flag(tp, USE_PHYLIB)) {
> > int i;
> >  
> > -   tg3_phy_lpbk_set(tp, 0, false);
> > -
> > +   err = tg3_phy_lpbk_set(tp, 0, false);
> > +   if (err)
> > +   goto done;
> > +
> > /* Wait for link */
> > for (i = 0; i < 100; i++) {
> > if (tr32(MAC_TX_STATUS) & TX_STATUS_LINK_UP)
> > @@ -13644,8 +13646,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
> > *data, bool do_extlpbk)
> > data[TG3_PHY_LOOPB_TEST] |= TG3_JMB_LOOPBACK_FAILED;
> >  
> > if (do_extlpbk) {
> > -   tg3_phy_lpbk_set(tp, 0, true);
> > -
> > +   err = tg3_phy_lpbk_set(tp, 0, true);
> > +
> > +   if (err)
> > +   goto done;
> > /* All link indications report up, but the hardware
> >  * isn't really ready for about 20 msec.  Double it
> >  * to be sure.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tg3:Add error handling to the function tg3_test_loopback

[Michael Chan]

On Thu, 2015-07-16 at 14:51 -0400, Nicholas Krause wrote: 
> This adds proper error handling for if the calls to the function
> tg3_phy_lpbk_set fail by returning -EIO by assigning the return
> value to the variable err and if it equals anything other then
> zero jumps to the goto label done as no other work can be handled
> internally in the function tg3_test_loopback.
> 
> Signed-off-by: Nicholas Krause 

Acked-by: Michael Chan 

> ---
>  drivers/net/ethernet/broadcom/tg3.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> b/drivers/net/ethernet/broadcom/tg3.c
> index 73c934c..8584cb7 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -13625,8 +13625,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
> *data, bool do_extlpbk)
>   !tg3_flag(tp, USE_PHYLIB)) {
>   int i;
>  
> - tg3_phy_lpbk_set(tp, 0, false);
> -
> + err = tg3_phy_lpbk_set(tp, 0, false);
> + if (err)
> + goto done;
> +
>   /* Wait for link */
>   for (i = 0; i < 100; i++) {
>   if (tr32(MAC_TX_STATUS) & TX_STATUS_LINK_UP)
> @@ -13644,8 +13646,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
> *data, bool do_extlpbk)
>   data[TG3_PHY_LOOPB_TEST] |= TG3_JMB_LOOPBACK_FAILED;
>  
>   if (do_extlpbk) {
> - tg3_phy_lpbk_set(tp, 0, true);
> -
> + err = tg3_phy_lpbk_set(tp, 0, true);
> +
> + if (err)
> + goto done;
>   /* All link indications report up, but the hardware
>* isn't really ready for about 20 msec.  Double it
>* to be sure.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tg3:Add error handling to the function tg3_test_loopback

[Michael Chan]

On Thu, 2015-07-16 at 12:18 -0700, Michael Chan wrote: 
 On Thu, 2015-07-16 at 14:51 -0400, Nicholas Krause wrote: 
  This adds proper error handling for if the calls to the function
  tg3_phy_lpbk_set fail by returning -EIO by assigning the return
  value to the variable err and if it equals anything other then
  zero jumps to the goto label done as no other work can be handled
  internally in the function tg3_test_loopback.
  
  Signed-off-by: Nicholas Krause xerofo...@gmail.com
 
 Acked-by: Michael Chan mc...@broadcom.com

Your indentation doesn't look right.  Other than that, it is ok.

 
  ---
   drivers/net/ethernet/broadcom/tg3.c | 12 
   1 file changed, 8 insertions(+), 4 deletions(-)
  
  diff --git a/drivers/net/ethernet/broadcom/tg3.c 
  b/drivers/net/ethernet/broadcom/tg3.c
  index 73c934c..8584cb7 100644
  --- a/drivers/net/ethernet/broadcom/tg3.c
  +++ b/drivers/net/ethernet/broadcom/tg3.c
  @@ -13625,8 +13625,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
  *data, bool do_extlpbk)
  !tg3_flag(tp, USE_PHYLIB)) {
  int i;
   
  -   tg3_phy_lpbk_set(tp, 0, false);
  -
  +   err = tg3_phy_lpbk_set(tp, 0, false);
  +   if (err)
  +   goto done;
  +
  /* Wait for link */
  for (i = 0; i  100; i++) {
  if (tr32(MAC_TX_STATUS)  TX_STATUS_LINK_UP)
  @@ -13644,8 +13646,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
  *data, bool do_extlpbk)
  data[TG3_PHY_LOOPB_TEST] |= TG3_JMB_LOOPBACK_FAILED;
   
  if (do_extlpbk) {
  -   tg3_phy_lpbk_set(tp, 0, true);
  -
  +   err = tg3_phy_lpbk_set(tp, 0, true);
  +
  +   if (err)
  +   goto done;
  /* All link indications report up, but the hardware
   * isn't really ready for about 20 msec.  Double it
   * to be sure.
 


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tg3:Add error handling to the function tg3_test_loopback

[Michael Chan]

On Thu, 2015-07-16 at 14:51 -0400, Nicholas Krause wrote: 
 This adds proper error handling for if the calls to the function
 tg3_phy_lpbk_set fail by returning -EIO by assigning the return
 value to the variable err and if it equals anything other then
 zero jumps to the goto label done as no other work can be handled
 internally in the function tg3_test_loopback.
 
 Signed-off-by: Nicholas Krause xerofo...@gmail.com

Acked-by: Michael Chan mc...@broadcom.com

 ---
  drivers/net/ethernet/broadcom/tg3.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/net/ethernet/broadcom/tg3.c 
 b/drivers/net/ethernet/broadcom/tg3.c
 index 73c934c..8584cb7 100644
 --- a/drivers/net/ethernet/broadcom/tg3.c
 +++ b/drivers/net/ethernet/broadcom/tg3.c
 @@ -13625,8 +13625,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
 *data, bool do_extlpbk)
   !tg3_flag(tp, USE_PHYLIB)) {
   int i;
  
 - tg3_phy_lpbk_set(tp, 0, false);
 -
 + err = tg3_phy_lpbk_set(tp, 0, false);
 + if (err)
 + goto done;
 +
   /* Wait for link */
   for (i = 0; i  100; i++) {
   if (tr32(MAC_TX_STATUS)  TX_STATUS_LINK_UP)
 @@ -13644,8 +13646,10 @@ static int tg3_test_loopback(struct tg3 *tp, u64 
 *data, bool do_extlpbk)
   data[TG3_PHY_LOOPB_TEST] |= TG3_JMB_LOOPBACK_FAILED;
  
   if (do_extlpbk) {
 - tg3_phy_lpbk_set(tp, 0, true);
 -
 + err = tg3_phy_lpbk_set(tp, 0, true);
 +
 + if (err)
 + goto done;
   /* All link indications report up, but the hardware
* isn't really ready for about 20 msec.  Double it
* to be sure.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [3.19-rc3] tg3: BUG: sleeping function called from invalid context

[Michael Chan]

On Tue, 2015-01-13 at 07:47 -0500, Peter Hurley wrote: 
> > tp->lock is held in this code path.  If synchronize_irq() sleeps in
> > wait_event(desc->wait_for_threads, ...), we'll get the warning.
> > 
> > The synchronize_irq() call is to wait for any tg3 irq handler to finish
> > so that it is guaranteed that next time it will see the CHIP_RESETTING
> > flag and do nothing.
> > 
> > Not sure if we can drop the tp->lock before we call synchronize_irq()
> > and then take it again after synchronize_irq().
> 
> Well, this device [1] is using MSI (INTx disabled) so if the synchronize_irq()
> is _only_ for the CHIP_RESETTING logic then it would seem ok to skip it (the
> synchronize_irq()). 

It is only for INTx.  But any device can operate in INTx mode if
MSI/MSIX is not available, so the fix needs to work in all cases.

Let me review the code some more.  If we can guarantee that another
reset, the timer code, etc, cannot come in even if we drop the tp->lock,
the simplest fix will be to drop it before calling synchronize_irq().

Thanks. 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [3.19-rc3] tg3: BUG: sleeping function called from invalid context

[Michael Chan]

On Tue, 2015-01-13 at 07:47 -0500, Peter Hurley wrote: 
  tp-lock is held in this code path.  If synchronize_irq() sleeps in
  wait_event(desc-wait_for_threads, ...), we'll get the warning.
  
  The synchronize_irq() call is to wait for any tg3 irq handler to finish
  so that it is guaranteed that next time it will see the CHIP_RESETTING
  flag and do nothing.
  
  Not sure if we can drop the tp-lock before we call synchronize_irq()
  and then take it again after synchronize_irq().
 
 Well, this device [1] is using MSI (INTx disabled) so if the synchronize_irq()
 is _only_ for the CHIP_RESETTING logic then it would seem ok to skip it (the
 synchronize_irq()). 

It is only for INTx.  But any device can operate in INTx mode if
MSI/MSIX is not available, so the fix needs to work in all cases.

Let me review the code some more.  If we can guarantee that another
reset, the timer code, etc, cannot come in even if we drop the tp-lock,
the simplest fix will be to drop it before calling synchronize_irq().

Thanks. 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [3.19-rc3] tg3: BUG: sleeping function called from invalid context

[Michael Chan]

On Mon, 2015-01-12 at 19:59 -0500, Peter Hurley wrote: 
> [   17.203009] BUG: sleeping function called from invalid context at 
> /home/peter/src/kernels/mainline/kernel/irq/manage.c:104
> [   17.203067] in_atomic(): 1, irqs_disabled(): 0, pid: 1106, name: ip
> [   17.203092] 2 locks held by ip/1106:
> [   17.205255]  #0:  (rtnl_mutex){+.+.+.}, at: [] 
> rtnetlink_rcv+0x1f/0x40
> [   17.207445]  #1:  (&(>lock)->rlock){+.}, at: [] 
> tg3_start+0xc06/0x11f0 [tg3]
> [   17.209725] CPU: 2 PID: 1106 Comm: ip Not tainted 
> 3.19.0-rc3+wip-xeon+lockdep #rc3+wip
> [   17.211900] Hardware name: Dell Inc. Precision WorkStation T5400  /0RW203, 
> BIOS A11 04/30/2012
> [   17.214086]  0068 8802ac823498 817af7e8 
> 0005
> [   17.216265]  81a9be78 8802ac8234a8 810998a5 
> 8802ac8234d8
> [   17.218446]  8109991a 8802ac8234c8 8802af0aae00 
> a00ed000
> [   17.220636] Call Trace:
> [   17.222743]  [] dump_stack+0x4f/0x7b
> [   17.224808]  [] ___might_sleep+0x105/0x140
> [   17.226842]  [] __might_sleep+0x3a/0xa0
> [   17.228869]  [] ? 0xa00ed000
> [   17.230939]  [] synchronize_irq+0x38/0xa0
> [   17.232967]  [] ? 0xa00ed000
> [   17.234991]  [] tg3_chip_reset+0x13f/0x9c0 [tg3]
> [   17.236988]  [] tg3_reset_hw+0x7e/0x2d20 [tg3] 

tp->lock is held in this code path.  If synchronize_irq() sleeps in
wait_event(desc->wait_for_threads, ...), we'll get the warning.

The synchronize_irq() call is to wait for any tg3 irq handler to finish
so that it is guaranteed that next time it will see the CHIP_RESETTING
flag and do nothing.

Not sure if we can drop the tp->lock before we call synchronize_irq()
and then take it again after synchronize_irq().

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [3.19-rc3] tg3: BUG: sleeping function called from invalid context

[Michael Chan]

On Mon, 2015-01-12 at 19:59 -0500, Peter Hurley wrote: 
 [   17.203009] BUG: sleeping function called from invalid context at 
 /home/peter/src/kernels/mainline/kernel/irq/manage.c:104
 [   17.203067] in_atomic(): 1, irqs_disabled(): 0, pid: 1106, name: ip
 [   17.203092] 2 locks held by ip/1106:
 [   17.205255]  #0:  (rtnl_mutex){+.+.+.}, at: [816adf1f] 
 rtnetlink_rcv+0x1f/0x40
 [   17.207445]  #1:  ((tp-lock)-rlock){+.}, at: [a01073e6] 
 tg3_start+0xc06/0x11f0 [tg3]
 [   17.209725] CPU: 2 PID: 1106 Comm: ip Not tainted 
 3.19.0-rc3+wip-xeon+lockdep #rc3+wip
 [   17.211900] Hardware name: Dell Inc. Precision WorkStation T5400  /0RW203, 
 BIOS A11 04/30/2012
 [   17.214086]  0068 8802ac823498 817af7e8 
 0005
 [   17.216265]  81a9be78 8802ac8234a8 810998a5 
 8802ac8234d8
 [   17.218446]  8109991a 8802ac8234c8 8802af0aae00 
 a00ed000
 [   17.220636] Call Trace:
 [   17.222743]  [817af7e8] dump_stack+0x4f/0x7b
 [   17.224808]  [810998a5] ___might_sleep+0x105/0x140
 [   17.226842]  [8109991a] __might_sleep+0x3a/0xa0
 [   17.228869]  [a00ed000] ? 0xa00ed000
 [   17.230939]  [810d7d78] synchronize_irq+0x38/0xa0
 [   17.232967]  [a00ed000] ? 0xa00ed000
 [   17.234991]  [a010105f] tg3_chip_reset+0x13f/0x9c0 [tg3]
 [   17.236988]  [a01020ae] tg3_reset_hw+0x7e/0x2d20 [tg3] 

tp-lock is held in this code path.  If synchronize_irq() sleeps in
wait_event(desc-wait_for_threads, ...), we'll get the warning.

The synchronize_irq() call is to wait for any tg3 irq handler to finish
so that it is guaranteed that next time it will see the CHIP_RESETTING
flag and do nothing.

Not sure if we can drop the tp-lock before we call synchronize_irq()
and then take it again after synchronize_irq().

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c

[Michael Chan]

On Tue, 2014-09-09 at 23:16 +, Anish Bhatt wrote: 
> > It would be really good if SCSI_NETLINK depended on NET instead of selected 
> > NET.
> > We shouldn't have kconfig symbols that use 'select' on entire subsystems.
> 
> As a test, I was able to fix this by this approach : change SCSI_NETLINK to 
> depend  
> on NET instead of selecting NET, and replacing 'select NETDEVICES ETHERNET 
> NET_VENDOR_BROADCOM CNIC' to 'depends on CNIC' in bnx2i & bnx2fc. (as CNIC 
> already has a dependency on those). This removes the need to check for IPV6 
> in 
> bnx2i/fc Kconfigs as well.
> 
> The side effect is that you won't see BNX2 drivers unless CNIC is selected, 
> similar
> for SCSI_NETLINK as well, not sure if there are any other implications.

Right.  I think it is more user-friendly for BNX2I and BNX2FC to select
CNIC.  CNIC is not useful by itself and it makes more sense to select it
automatically when BNX2I/BNX2FC are enabled.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c

[Michael Chan]

On Tue, 2014-09-09 at 20:24 +, Anish Bhatt wrote: 
> This is caused by c99d667e8527  ("cnic : Cleanup CONFIG_IPV6 & VLAN check")
> 
> So I'm not really sure what the fix for this is. CNIC will properly only 
> support 
> [m] or [n] when IPV6 is compiled as a module, but if you set SCSI_BNX2X_FCOE 
> or 
> SCSI_BNX2_ISCSI to be in built, there is a 'select CNIC' that forces CNIC to 
> be 
> inbuilt, even though this is not supported by Kconfig. I was under the 
> assumption that select will not forcibly change m to inbuilt (or perhaps I'm 
> missing some syntax suboption ?)
> 
> This is still fixable in SCSI_BNX2_ISCSI using the same logic used in the 
> above 
> mentioned commit, but not so in SCSI_BNX2X_FCOE where it causes a recursive 
> dependency error (thought it is fixing the issue nevertheless), and none of 
> the 
>  other solutions I tried seemed to work either. 

Adding depends on IPV6 || IPV6=n doesn't work for SCSI_BNX2X_FCOE, but
works for SCSI_BNX2_ISCSI?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c

[Michael Chan]

On Tue, 2014-09-09 at 20:24 +, Anish Bhatt wrote: 
 This is caused by c99d667e8527  (cnic : Cleanup CONFIG_IPV6  VLAN check)
 
 So I'm not really sure what the fix for this is. CNIC will properly only 
 support 
 [m] or [n] when IPV6 is compiled as a module, but if you set SCSI_BNX2X_FCOE 
 or 
 SCSI_BNX2_ISCSI to be in built, there is a 'select CNIC' that forces CNIC to 
 be 
 inbuilt, even though this is not supported by Kconfig. I was under the 
 assumption that select will not forcibly change m to inbuilt (or perhaps I'm 
 missing some syntax suboption ?)
 
 This is still fixable in SCSI_BNX2_ISCSI using the same logic used in the 
 above 
 mentioned commit, but not so in SCSI_BNX2X_FCOE where it causes a recursive 
 dependency error (thought it is fixing the issue nevertheless), and none of 
 the 
  other solutions I tried seemed to work either. 

Adding depends on IPV6 || IPV6=n doesn't work for SCSI_BNX2X_FCOE, but
works for SCSI_BNX2_ISCSI?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c

[Michael Chan]

On Tue, 2014-09-09 at 23:16 +, Anish Bhatt wrote: 
  It would be really good if SCSI_NETLINK depended on NET instead of selected 
  NET.
  We shouldn't have kconfig symbols that use 'select' on entire subsystems.
 
 As a test, I was able to fix this by this approach : change SCSI_NETLINK to 
 depend  
 on NET instead of selecting NET, and replacing 'select NETDEVICES ETHERNET 
 NET_VENDOR_BROADCOM CNIC' to 'depends on CNIC' in bnx2i  bnx2fc. (as CNIC 
 already has a dependency on those). This removes the need to check for IPV6 
 in 
 bnx2i/fc Kconfigs as well.
 
 The side effect is that you won't see BNX2 drivers unless CNIC is selected, 
 similar
 for SCSI_NETLINK as well, not sure if there are any other implications.

Right.  I think it is more user-friendly for BNX2I and BNX2FC to select
CNIC.  CNIC is not useful by itself and it makes more sense to select it
automatically when BNX2I/BNX2FC are enabled.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold

[Michael Chan]

On Thu, 2014-08-21 at 16:06 -0700, Benjamin Poirier wrote: 
> On 2014/08/21 15:32, Michael Chan wrote:
> > On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote: 
> > > On 2014/08/19 15:00, Michael Chan wrote:
> > > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
> > > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> > > > > b/drivers/net/ethernet/broadcom/tg3.c
> > > > > index 3ac5d23..b11c0fd 100644
> > > > > --- a/drivers/net/ethernet/broadcom/tg3.c
> > > > > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > > > > @@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS 
> > > > > flag, unsigned long *bits)
> > > > >  #endif
> > > > >  
> > > > >  /* minimum number of free TX descriptors required to wake up TX 
> > > > > process */
> > > > > -#define TG3_TX_WAKEUP_THRESH(tnapi)((tnapi)->tx_pending 
> > > > > / 4)
> > > > > +#define TG3_TX_WAKEUP_THRESH(tnapi)max_t(u32, 
> > > > > (tnapi)->tx_pending / 4, \
> > > > > + MAX_SKB_FRAGS + 1)
> > > > 
> > > > I think we should precompute this and store it in something like
> > > > tp->tx_wake_thresh.
> > > 
> > > I've tried this by adding the following patch at the end of the v2
> > > series but I did not measure a significant latency improvement. Was
> > > there another reason for the change? 
> > 
> > Just performance.  The wake up threshold is checked in the tx fast path
> > in both start_xmit() and tg3_tx().  I would optimize such code for speed
> 
> I don't see what you mean. The code in those two functions that used to
> invoke TG3_TX_WAKEUP_THRESH is wrapped in unlikely() conditions. You
> can't tell me that's the fast path ;) It's only checked when the queue
> is stopped.

I missed the unlikely().  So you're right.  It's not really in the fast
path.

> 
> Moreover, the patches I've sent already add tg3_napi.wakeup_thresh. It
> is over those patches that I've made the measurements.

Right.  But my original comment was over your original patch #1 which
was adding max_t() to the macro TG3_TX_WAKE_THRESH without adding
wakeup_thresh field.  All my comments (performance and smaller code)
were based on your original patch #1.  Later I did see that your patch 3
converted TG3_TX_WAKEUP_THRESH to a structure field so it's no longer an
issue.

> 
> > as much as possible.  In the current code, it was just a right shift
> > operation.  Now, with max_t() added, I think I prefer having it
> > pre-computed.  The performance difference may not be measurable, but I
> > think the compiled code size may be smaller too.
> 
> Maybe in certain areas, but not overall:
> 
> with v2 patches 1-3
>textdata bss dec hex filename
>  1494951247   0  150742   24cd6 drivers/net/ethernet/broadcom/tg3.o
> with v2 patches 1-3 + tx_wake_thresh_def
>textdata bss dec hex filename
>  1495241247   0  150771   24cf3 drivers/net/ethernet/broadcom/tg3.o
> 
> I really don't see a gain.
> 

Agreed.  Once you have converted the TG3_TX_WAKEUP_THRESH to a structure
field, that's sufficient.  No need to have multiple fields.  Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold

[Michael Chan]

On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote: 
> On 2014/08/19 15:00, Michael Chan wrote:
> > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
> > > diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> > > b/drivers/net/ethernet/broadcom/tg3.c
> > > index 3ac5d23..b11c0fd 100644
> > > --- a/drivers/net/ethernet/broadcom/tg3.c
> > > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > > @@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS 
> > > flag, unsigned long *bits)
> > >  #endif
> > >  
> > >  /* minimum number of free TX descriptors required to wake up TX process 
> > > */
> > > -#define TG3_TX_WAKEUP_THRESH(tnapi)((tnapi)->tx_pending / 4)
> > > +#define TG3_TX_WAKEUP_THRESH(tnapi)max_t(u32, (tnapi)->tx_pending / 
> > > 4, \
> > > + MAX_SKB_FRAGS + 1)
> > 
> > I think we should precompute this and store it in something like
> > tp->tx_wake_thresh.
> 
> I've tried this by adding the following patch at the end of the v2
> series but I did not measure a significant latency improvement. Was
> there another reason for the change? 

Just performance.  The wake up threshold is checked in the tx fast path
in both start_xmit() and tg3_tx().  I would optimize such code for speed
as much as possible.  In the current code, it was just a right shift
operation.  Now, with max_t() added, I think I prefer having it
pre-computed.  The performance difference may not be measurable, but I
think the compiled code size may be smaller too.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] tg3: Fix tx_pending checks for tg3_tso_bug

[Michael Chan]

On Wed, 2014-08-20 at 18:23 -0700, Benjamin Poirier wrote: 
> On 2014/08/19 16:10, Michael Chan wrote:
> > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
> > > @@ -7838,11 +7838,14 @@ static int tg3_tso_bug(struct tg3 *tp, struct 
> > > tg3_napi *tnapi,
> > >struct netdev_queue *txq, struct sk_buff *skb)
> > >  {
> > > struct sk_buff *segs, *nskb;
> > > -   u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
> > >  
> > > -   /* Estimate the number of fragments in the worst case */
> > > -   if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
> > > +   if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) {
> > > +   trace_printk("stopping queue, %d <= %d\n",
> > > +tg3_tx_avail(tnapi), 
> > > skb_shinfo(skb)->gso_segs);
> > > netif_tx_stop_queue(txq);
> > > +   trace_printk("stopped queue\n");
> > > +   tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs;
> > > +   BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending);
> > >  
> > > /* netif_tx_stop_queue() must be done before checking
> > >  * checking tx index in tg3_tx_avail() below, because in 
> > 
> > I don't quite understand this logic and I must be missing something.
> > gso_segs is the number of TCP segments the large packet will be broken
> > up into.  If it exceeds dev->gso_max_segs, it means it exceeds
> > hardware's capabilty and it will do GSO instead of TSO.  But in this
> > case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA
> > descriptors to do GSO.  Each gso_seg typically requires 2 DMA
> > descriptors.
> 
> You're right, I had wrongly assumed that the skbs coming out of
> skb_gso_segment() were linear. I'll address that in v2 of the patch by masking
> out NETIF_F_SG in tg3_tso_bug().
> 

While masking out NETF_F_SG will work, it will also disable checksum
offload for the whole device momentarily.

> I noticed another issue that had not occurred to me: when tg3_tso_bug is
> submitting a full gso segs sequence to tg3_start_xmit, the code at the end of
> that function stops the queue before the end of the sequence because tx_avail
> becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds
> because tg3_tso_bug() does not honour the queue state but it seems rather
> unsightly to me.

That's why the number of DMA descriptors that we estimate has to be
accurate.  It's unfortunate that the various tg3 chips require so many
different workarounds.  The objective is to keep TSO and checksum
enabled and workaround the occasional packets using GSO.

I believe that the boundary error conditions that you brought up can be
addressed by enforcing some limits on the tx ring size and by reducing
gso_max_size/gso_max_segs when necessary (for example when MTU and/or
ring size is set very small).



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] tg3: Fix tx_pending checks for tg3_tso_bug

[Michael Chan]

On Wed, 2014-08-20 at 18:23 -0700, Benjamin Poirier wrote: 
 On 2014/08/19 16:10, Michael Chan wrote:
  On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
   @@ -7838,11 +7838,14 @@ static int tg3_tso_bug(struct tg3 *tp, struct 
   tg3_napi *tnapi,
  struct netdev_queue *txq, struct sk_buff *skb)
{
   struct sk_buff *segs, *nskb;
   -   u32 frag_cnt_est = skb_shinfo(skb)-gso_segs * 3;

   -   /* Estimate the number of fragments in the worst case */
   -   if (unlikely(tg3_tx_avail(tnapi) = frag_cnt_est)) {
   +   if (unlikely(tg3_tx_avail(tnapi) = skb_shinfo(skb)-gso_segs)) {
   +   trace_printk(stopping queue, %d = %d\n,
   +tg3_tx_avail(tnapi), 
   skb_shinfo(skb)-gso_segs);
   netif_tx_stop_queue(txq);
   +   trace_printk(stopped queue\n);
   +   tnapi-wakeup_thresh = skb_shinfo(skb)-gso_segs;
   +   BUG_ON(tnapi-wakeup_thresh = tnapi-tx_pending);

   /* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in 
  
  I don't quite understand this logic and I must be missing something.
  gso_segs is the number of TCP segments the large packet will be broken
  up into.  If it exceeds dev-gso_max_segs, it means it exceeds
  hardware's capabilty and it will do GSO instead of TSO.  But in this
  case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA
  descriptors to do GSO.  Each gso_seg typically requires 2 DMA
  descriptors.
 
 You're right, I had wrongly assumed that the skbs coming out of
 skb_gso_segment() were linear. I'll address that in v2 of the patch by masking
 out NETIF_F_SG in tg3_tso_bug().
 

While masking out NETF_F_SG will work, it will also disable checksum
offload for the whole device momentarily.

 I noticed another issue that had not occurred to me: when tg3_tso_bug is
 submitting a full gso segs sequence to tg3_start_xmit, the code at the end of
 that function stops the queue before the end of the sequence because tx_avail
 becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds
 because tg3_tso_bug() does not honour the queue state but it seems rather
 unsightly to me.

That's why the number of DMA descriptors that we estimate has to be
accurate.  It's unfortunate that the various tg3 chips require so many
different workarounds.  The objective is to keep TSO and checksum
enabled and workaround the occasional packets using GSO.

I believe that the boundary error conditions that you brought up can be
addressed by enforcing some limits on the tx ring size and by reducing
gso_max_size/gso_max_segs when necessary (for example when MTU and/or
ring size is set very small).



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold

[Michael Chan]

On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote: 
 On 2014/08/19 15:00, Michael Chan wrote:
  On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
   diff --git a/drivers/net/ethernet/broadcom/tg3.c 
   b/drivers/net/ethernet/broadcom/tg3.c
   index 3ac5d23..b11c0fd 100644
   --- a/drivers/net/ethernet/broadcom/tg3.c
   +++ b/drivers/net/ethernet/broadcom/tg3.c
   @@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS 
   flag, unsigned long *bits)
#endif

/* minimum number of free TX descriptors required to wake up TX process 
   */
   -#define TG3_TX_WAKEUP_THRESH(tnapi)((tnapi)-tx_pending / 4)
   +#define TG3_TX_WAKEUP_THRESH(tnapi)max_t(u32, (tnapi)-tx_pending / 
   4, \
   + MAX_SKB_FRAGS + 1)
  
  I think we should precompute this and store it in something like
  tp-tx_wake_thresh.
 
 I've tried this by adding the following patch at the end of the v2
 series but I did not measure a significant latency improvement. Was
 there another reason for the change? 

Just performance.  The wake up threshold is checked in the tx fast path
in both start_xmit() and tg3_tx().  I would optimize such code for speed
as much as possible.  In the current code, it was just a right shift
operation.  Now, with max_t() added, I think I prefer having it
pre-computed.  The performance difference may not be measurable, but I
think the compiled code size may be smaller too.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold

[Michael Chan]

On Thu, 2014-08-21 at 16:06 -0700, Benjamin Poirier wrote: 
 On 2014/08/21 15:32, Michael Chan wrote:
  On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote: 
   On 2014/08/19 15:00, Michael Chan wrote:
On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
 diff --git a/drivers/net/ethernet/broadcom/tg3.c 
 b/drivers/net/ethernet/broadcom/tg3.c
 index 3ac5d23..b11c0fd 100644
 --- a/drivers/net/ethernet/broadcom/tg3.c
 +++ b/drivers/net/ethernet/broadcom/tg3.c
 @@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS 
 flag, unsigned long *bits)
  #endif
  
  /* minimum number of free TX descriptors required to wake up TX 
 process */
 -#define TG3_TX_WAKEUP_THRESH(tnapi)((tnapi)-tx_pending 
 / 4)
 +#define TG3_TX_WAKEUP_THRESH(tnapi)max_t(u32, 
 (tnapi)-tx_pending / 4, \
 + MAX_SKB_FRAGS + 1)

I think we should precompute this and store it in something like
tp-tx_wake_thresh.
   
   I've tried this by adding the following patch at the end of the v2
   series but I did not measure a significant latency improvement. Was
   there another reason for the change? 
  
  Just performance.  The wake up threshold is checked in the tx fast path
  in both start_xmit() and tg3_tx().  I would optimize such code for speed
 
 I don't see what you mean. The code in those two functions that used to
 invoke TG3_TX_WAKEUP_THRESH is wrapped in unlikely() conditions. You
 can't tell me that's the fast path ;) It's only checked when the queue
 is stopped.

I missed the unlikely().  So you're right.  It's not really in the fast
path.

 
 Moreover, the patches I've sent already add tg3_napi.wakeup_thresh. It
 is over those patches that I've made the measurements.

Right.  But my original comment was over your original patch #1 which
was adding max_t() to the macro TG3_TX_WAKE_THRESH without adding
wakeup_thresh field.  All my comments (performance and smaller code)
were based on your original patch #1.  Later I did see that your patch 3
converted TG3_TX_WAKEUP_THRESH to a structure field so it's no longer an
issue.

 
  as much as possible.  In the current code, it was just a right shift
  operation.  Now, with max_t() added, I think I prefer having it
  pre-computed.  The performance difference may not be measurable, but I
  think the compiled code size may be smaller too.
 
 Maybe in certain areas, but not overall:
 
 with v2 patches 1-3
textdata bss dec hex filename
  1494951247   0  150742   24cd6 drivers/net/ethernet/broadcom/tg3.o
 with v2 patches 1-3 + tx_wake_thresh_def
textdata bss dec hex filename
  1495241247   0  150771   24cf3 drivers/net/ethernet/broadcom/tg3.o
 
 I really don't see a gain.
 

Agreed.  Once you have converted the TG3_TX_WAKEUP_THRESH to a structure
field, that's sufficient.  No need to have multiple fields.  Thanks.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] tg3: Fix tx_pending checks for tg3_tso_bug

[Michael Chan]

On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
> @@ -7838,11 +7838,14 @@ static int tg3_tso_bug(struct tg3 *tp, struct 
> tg3_napi *tnapi,
>struct netdev_queue *txq, struct sk_buff *skb)
>  {
> struct sk_buff *segs, *nskb;
> -   u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
>  
> -   /* Estimate the number of fragments in the worst case */
> -   if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
> +   if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) {
> +   trace_printk("stopping queue, %d <= %d\n",
> +tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs);
> netif_tx_stop_queue(txq);
> +   trace_printk("stopped queue\n");
> +   tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs;
> +   BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending);
>  
> /* netif_tx_stop_queue() must be done before checking
>  * checking tx index in tg3_tx_avail() below, because in 

I don't quite understand this logic and I must be missing something.
gso_segs is the number of TCP segments the large packet will be broken
up into.  If it exceeds dev->gso_max_segs, it means it exceeds
hardware's capabilty and it will do GSO instead of TSO.  But in this
case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA
descriptors to do GSO.  Each gso_seg typically requires 2 DMA
descriptors.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold

[Michael Chan]

On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c 
> b/drivers/net/ethernet/broadcom/tg3.c
> index 3ac5d23..b11c0fd 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS flag, 
> unsigned long *bits)
>  #endif
>  
>  /* minimum number of free TX descriptors required to wake up TX process */
> -#define TG3_TX_WAKEUP_THRESH(tnapi)((tnapi)->tx_pending / 4)
> +#define TG3_TX_WAKEUP_THRESH(tnapi)max_t(u32, (tnapi)->tx_pending / 4, \
> + MAX_SKB_FRAGS + 1)

I think we should precompute this and store it in something like
tp->tx_wake_thresh.

>  #define TG3_TX_BD_DMA_MAX_2K   2048
>  #define TG3_TX_BD_DMA_MAX_4K   4096
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/