| Message ID | 1387557965-13241-12-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] > Sent: Friday, December 20, 2013 10:18 AM > To: Kirsher, Jeffrey T; davem@davemloft.net; Williams, Mitch A > Cc: netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; > Brandeburg, Jesse > Subject: Re: [net-next v2 11/16] i40e: remove chatty log messages > > On 12/20/2013 07:46 PM, Jeff Kirsher wrote: > > > From: Mitch Williams <mitch.a.williams@intel.com> > > > Don't complain when we disable queues that are already disable, or > > enable them when they're already enabled. This removes a bunch of bogus > > log messages that we see at every VF reset. > > > Change-Id: Ia127be572abdccc48a53d8c43f8a07b8bb920de1 > > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > --- > > drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++------------- > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index c0f78bc..38e07e6 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi > *vsi, bool enable) > > } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) > > ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & 1); > > > > - if (enable) { > > - /* is STAT set ? */ > > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > - dev_info(&pf->pdev->dev, > > - "Tx %d already enabled\n", i); > > + /* Skip if the queue is already in the requested state */ > > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > continue; > > This line seems over-indented now. > > > - } > > - } else { > > - /* is !STAT set ? */ > > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > - dev_info(&pf->pdev->dev, > > - "Tx %d already disabled\n", i); > > + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > continue; > > This one too. > > WBR, Sergei Sergei, if you look at the source instead of the patch, you'll see that these are correct. The whole thing is inside a for loop, so it should properly be indented two tabs. -Mitch -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-12-20 at 17:40 +0000, Williams, Mitch A wrote: > > -----Original Message----- > > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] [] > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c [] > > > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi > > *vsi, bool enable) > > > } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) > > > ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & 1); > > > > > > - if (enable) { > > > - /* is STAT set ? */ > > > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > - dev_info(&pf->pdev->dev, > > > - "Tx %d already enabled\n", i); > > > + /* Skip if the queue is already in the requested state */ > > > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > continue; > > > > This line seems over-indented now. > > > > > - } > > > - } else { > > > - /* is !STAT set ? */ > > > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > - dev_info(&pf->pdev->dev, > > > - "Tx %d already disabled\n", i); > > > + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > continue; > > > > This one too. [] > Sergei, if you look at the source instead of the patch, you'll see > that these are correct. The whole thing is inside a for loop, so it > should properly be indented two tabs. I looked at the source. Both continue statements _are_ overly indented. 4 tabs should be 3. Also, this code is inconsistent and might be nicer using the same form: /* Skip if the queue is already in the requested state */ if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) continue; if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) continue; [...] /* wait for the change to finish */ for (j = 0; j < 10; j++) { tx_reg = rd32(hw, I40E_QTX_ENA(pf_q)); if (enable) { if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) break; } else { if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) break; } Perhaps the first form should be like the second if (enable) { if (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK) continue; } else { if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) continue; } or maybe both should be bool mask = tx_reg & I40E_QTX_ENA_QENA_STAT_MASK; if (enable ^ mask) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > Behalf Of Williams, Mitch A > Sent: Friday, December 20, 2013 9:40 AM > To: Sergei Shtylyov; Kirsher, Jeffrey T; davem@davemloft.net > Cc: netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; > Brandeburg, Jesse > Subject: RE: [net-next v2 11/16] i40e: remove chatty log messages > > > > > -----Original Message----- > > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] > > Sent: Friday, December 20, 2013 10:18 AM > > To: Kirsher, Jeffrey T; davem@davemloft.net; Williams, Mitch A > > Cc: netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; > > Brandeburg, Jesse > > Subject: Re: [net-next v2 11/16] i40e: remove chatty log messages > > > > On 12/20/2013 07:46 PM, Jeff Kirsher wrote: > > > > > From: Mitch Williams <mitch.a.williams@intel.com> > > > > > Don't complain when we disable queues that are already disable, or > > > enable them when they're already enabled. This removes a bunch of bogus > > > log messages that we see at every VF reset. > > > > > Change-Id: Ia127be572abdccc48a53d8c43f8a07b8bb920de1 > > > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> > > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > > Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com> > > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > > --- > > > drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++------------- > > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > > b/drivers/net/ethernet/intel/i40e/i40e_main.c > > > index c0f78bc..38e07e6 100644 > > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi > > *vsi, bool enable) > > > } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) > > > ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & > 1); > > > > > > - if (enable) { > > > - /* is STAT set ? */ > > > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > - dev_info(&pf->pdev->dev, > > > - "Tx %d already enabled\n", i); > > > + /* Skip if the queue is already in the requested state */ > > > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > continue; > > > > This line seems over-indented now. > > > > > - } > > > - } else { > > > - /* is !STAT set ? */ > > > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > - dev_info(&pf->pdev->dev, > > > - "Tx %d already disabled\n", i); > > > + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > continue; > > > > This one too. > > > > WBR, Sergei > > Sergei, if you look at the source instead of the patch, you'll see that > these are correct. The whole thing is inside a for loop, so it should > properly be indented two tabs. > > -Mitch Doh! Jeff pointed out to me that you are indeed correct, Sergei. I was looking at the if lines, not the continue lines. We'll fix this. -Mitch > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-12-20 at 09:59 -0800, Joe Perches wrote: > On Fri, 2013-12-20 at 17:40 +0000, Williams, Mitch A wrote: > > > -----Original Message----- > > > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] > [] > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > [] > > > > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct > i40e_vsi > > > *vsi, bool enable) > > > > } while (j-- && ((tx_reg >> > I40E_QTX_ENA_QENA_REQ_SHIFT) > > > > ^ (tx_reg >> > I40E_QTX_ENA_QENA_STAT_SHIFT)) & 1); > > > > > > > > - if (enable) { > > > > - /* is STAT set ? */ > > > > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > > - dev_info(&pf->pdev->dev, > > > > - "Tx %d already enabled\n", > i); > > > > + /* Skip if the queue is already in the requested state > */ > > > > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > > continue; > > > > > > This line seems over-indented now. > > > > > > > - } > > > > - } else { > > > > - /* is !STAT set ? */ > > > > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > > - dev_info(&pf->pdev->dev, > > > > - "Tx %d already disabled\n", > i); > > > > + if (!enable && !(tx_reg & > I40E_QTX_ENA_QENA_STAT_MASK)) > > > > continue; > > > > > > This one too. > [] > > Sergei, if you look at the source instead of the patch, you'll see > > that these are correct. The whole thing is inside a for loop, so it > > should properly be indented two tabs. > > I looked at the source. > Both continue statements _are_ overly indented. > 4 tabs should be 3. > > Also, this code is inconsistent and might be > nicer using the same form: > > /* Skip if the queue is already in the requested state > */ > if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; > if (!enable && !(tx_reg & > I40E_QTX_ENA_QENA_STAT_MASK)) > continue; Sergei was right. I will get this fixed up and re-submit v3.
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > Sent: Friday, December 20, 2013 9:59 AM > To: Williams, Mitch A > Cc: Sergei Shtylyov; Kirsher, Jeffrey T; davem@davemloft.net; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; Brandeburg, > Jesse > Subject: Re: [net-next v2 11/16] i40e: remove chatty log messages > > On Fri, 2013-12-20 at 17:40 +0000, Williams, Mitch A wrote: > > > -----Original Message----- > > > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] > [] > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > [] > > > > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi > > > *vsi, bool enable) > > > > } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) > > > > ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & > 1); > > > > > > > > - if (enable) { > > > > - /* is STAT set ? */ > > > > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > > - dev_info(&pf->pdev->dev, > > > > - "Tx %d already enabled\n", i); > > > > + /* Skip if the queue is already in the requested state */ > > > > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > > continue; > > > > > > This line seems over-indented now. > > > > > > > - } > > > > - } else { > > > > - /* is !STAT set ? */ > > > > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > > > > - dev_info(&pf->pdev->dev, > > > > - "Tx %d already disabled\n", i); > > > > + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > > > continue; > > > > > > This one too. > [] > > Sergei, if you look at the source instead of the patch, you'll see > > that these are correct. The whole thing is inside a for loop, so it > > should properly be indented two tabs. > > I looked at the source. > Both continue statements _are_ overly indented. > 4 tabs should be 3. > > Also, this code is inconsistent and might be > nicer using the same form: > > /* Skip if the queue is already in the requested state */ > if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; > if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; > > [...] > > /* wait for the change to finish */ > for (j = 0; j < 10; j++) { > tx_reg = rd32(hw, I40E_QTX_ENA(pf_q)); > if (enable) { > if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > break; > } else { > if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > break; > } > > Perhaps the first form should be like the second > > if (enable) { > if (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK) > continue; > } else { > if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; > } > > or maybe both should be > > bool mask = tx_reg & I40E_QTX_ENA_QENA_STAT_MASK; > if (enable ^ mask) > Yeah, I see the messed up indent now. Jeff's going to fix and resend. I see what you mean about the inconsistent logic. Since it's not crucial, I think I'd prefer to fix this in a separate patch in the future. (I'll implement it today, but it takes a while to get through our internal validation process.) -Mitch -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/20/2013 07:46 PM, Jeff Kirsher wrote: > From: Mitch Williams <mitch.a.williams@intel.com> > Don't complain when we disable queues that are already disable, or > enable them when they're already enabled. This removes a bunch of bogus > log messages that we see at every VF reset. > Change-Id: Ia127be572abdccc48a53d8c43f8a07b8bb920de1 > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index c0f78bc..38e07e6 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi *vsi, bool enable) > } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) > ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & 1); > > - if (enable) { > - /* is STAT set ? */ > - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > - dev_info(&pf->pdev->dev, > - "Tx %d already enabled\n", i); > + /* Skip if the queue is already in the requested state */ > + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; This line seems over-indented now. > - } > - } else { > - /* is !STAT set ? */ > - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { > - dev_info(&pf->pdev->dev, > - "Tx %d already disabled\n", i); > + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > continue; This one too. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-12-20 at 18:04 +0000, Williams, Mitch A wrote: > > From: Joe Perches [mailto:joe@perches.com] [] > > this code is inconsistent and might be > > nicer using the same form: > > > > /* Skip if the queue is already in the requested state */ > > if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > continue; > > if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > continue; > > > > [...] > > > > /* wait for the change to finish */ > > for (j = 0; j < 10; j++) { > > tx_reg = rd32(hw, I40E_QTX_ENA(pf_q)); > > if (enable) { > > if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > break; > > } else { > > if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > break; > > } > > > > Perhaps the first form should be like the second > > > > if (enable) { > > if (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK) > > continue; > > } else { > > if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) > > continue; > > } > > > > or maybe both should be > > > > bool mask = tx_reg & I40E_QTX_ENA_QENA_STAT_MASK; > > if (enable ^ mask) [] > I see what you mean about the inconsistent logic. Since it's not > crucial, I think I'd prefer to fix this in a separate patch in the > future. (I'll implement it today, but it takes a while to get through > our internal validation process.) Whenever, no worries, it's just a nit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index c0f78bc..38e07e6 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -2988,21 +2988,11 @@ static int i40e_vsi_control_tx(struct i40e_vsi *vsi, bool enable) } while (j-- && ((tx_reg >> I40E_QTX_ENA_QENA_REQ_SHIFT) ^ (tx_reg >> I40E_QTX_ENA_QENA_STAT_SHIFT)) & 1); - if (enable) { - /* is STAT set ? */ - if ((tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { - dev_info(&pf->pdev->dev, - "Tx %d already enabled\n", i); + /* Skip if the queue is already in the requested state */ + if (enable && (tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) continue; - } - } else { - /* is !STAT set ? */ - if (!(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) { - dev_info(&pf->pdev->dev, - "Tx %d already disabled\n", i); + if (!enable && !(tx_reg & I40E_QTX_ENA_QENA_STAT_MASK)) continue; - } - } /* turn on/off the queue */ if (enable)