Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Sun, 2015-02-08 at 00:21 -0500, Nicholas Krause wrote:
> Removes the outdated fix me comment in the
> function,gb_acquire_swfw_sync_i210
> for setting the variables, i and timeout to the intended correct
> values for
> the function,gb_acquire_swfw_sync_i210 to function correctly. This
> comment is
> no longer due to these values having been no changed in the last few
> years and
> no known issues have been found for these variables being set to their
> current
> values. Due to this the comment can now be removed as the values set
> for these
> variables are known to be correct after years of no known issues or
> bugs related
> to their current values.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_i210.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thank Nick, I will add your patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [E1000-devel] [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function, gb_acquire_swfw_sync_i210

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Sun, 2015-02-08 at 17:21 -0800, Jeff Kirsher wrote:
> On Sun, 2015-02-08 at 00:21 -0500, Nicholas Krause wrote:
> > Removes the outdated fix me comment in the
> > function,gb_acquire_swfw_sync_i210
> > for setting the variables, i and timeout to the intended correct
> > values for
> > the function,gb_acquire_swfw_sync_i210 to function correctly. This
> > comment is
> > no longer due to these values having been no changed in the last few
> > years and
> > no known issues have been found for these variables being set to their
> > current
> > values. Due to this the comment can now be removed as the values set
> > for these
> > variables are known to be correct after years of no known issues or
> > bugs related
> > to their current values.
> > 
> > Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> > ---
> >  drivers/net/ethernet/intel/igb/e1000_i210.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thank Nick, I will add your patch to my queue.

Oh and the title should be "igb: Remove outdated ...", not
"net:ethernet:intel: Remove ..."

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [E1000-devel] [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function, gb_acquire_swfw_sync_i210

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On Sun, 2015-02-08 at 20:28 -0500, nick wrote:
> 
> On 2015-02-08 08:26 PM, Jeff Kirsher wrote:
> > On Sun, 2015-02-08 at 17:21 -0800, Jeff Kirsher wrote:
> >> On Sun, 2015-02-08 at 00:21 -0500, Nicholas Krause wrote:
> >>> Removes the outdated fix me comment in the
> >>> function,gb_acquire_swfw_sync_i210
> >>> for setting the variables, i and timeout to the intended correct
> >>> values for
> >>> the function,gb_acquire_swfw_sync_i210 to function correctly. This
> >>> comment is
> >>> no longer due to these values having been no changed in the last few
> >>> years and
> >>> no known issues have been found for these variables being set to their
> >>> current
> >>> values. Due to this the comment can now be removed as the values set
> >>> for these
> >>> variables are known to be correct after years of no known issues or
> >>> bugs related
> >>> to their current values.
> >>>
> >>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >>> ---
> >>>  drivers/net/ethernet/intel/igb/e1000_i210.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Thank Nick, I will add your patch to my queue.
> > 
> > Oh and the title should be "igb: Remove outdated ...", not
> > "net:ethernet:intel: Remove ..."
> > 
> Jeff,
> Sorry about the title. Good to known for next time. Should I resend or 
> will you fix the title for me?
> Cheers,
> Nick

I will fix the title.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

[Andrej Manduch]

Hi,

Can you please leave full patch with git diff when you're replying to
Nicks e-mails? Nick is banned in *@vger.kernel.org so we cannot see his
patches and rest of the world is seeing only replyies to his e-mail. And
it's really hard to test anything when we don't see changes.

On 02/08/2015 08:21 PM, Jeff Kirsher wrote:
> On Sun, 2015-02-08 at 00:21 -0500, Nicholas Krause wrote:
>> Removes the outdated fix me comment in the
>> function,gb_acquire_swfw_sync_i210
>> for setting the variables, i and timeout to the intended correct
>> values for
>> the function,gb_acquire_swfw_sync_i210 to function correctly. This
>> comment is
>> no longer due to these values having been no changed in the last few
>> years and
>> no known issues have been found for these variables being set to their
>> current
>> values. Due to this the comment can now be removed as the values set
>> for these
>> variables are known to be correct after years of no known issues or
>> bugs related
>> to their current values.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/net/ethernet/intel/igb/e1000_i210.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thank Nick, I will add your patch to my queue.
> 

-- 
Kind regards,
b.

Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Mon, 2015-02-09 at 02:17 -0500, Andrej Manduch wrote:
> Hi,
> 
> Can you please leave full patch with git diff when you're replying to
> Nicks e-mails? Nick is banned in *@vger.kernel.org so we cannot see his
> patches and rest of the world is seeing only replyies to his e-mail. And
> it's really hard to test anything when we don't see changes.

Oh, I am well aware of Nick's current ban on @vger,kernel.org mailing
lists.  If you want to see Nick's patch, feel free to view his patch on
my queue tree:
https://git.kernel.org/cgit/linux/kernel/git/jkirsher/queue.git/

It is the top most patch (i.e. last one applied to my queue).

> 
> On 02/08/2015 08:21 PM, Jeff Kirsher wrote:
> > On Sun, 2015-02-08 at 00:21 -0500, Nicholas Krause wrote:
> >> Removes the outdated fix me comment in the
> >> function,gb_acquire_swfw_sync_i210
> >> for setting the variables, i and timeout to the intended correct
> >> values for
> >> the function,gb_acquire_swfw_sync_i210 to function correctly. This
> >> comment is
> >> no longer due to these values having been no changed in the last few
> >> years and
> >> no known issues have been found for these variables being set to their
> >> current
> >> values. Due to this the comment can now be removed as the values set
> >> for these
> >> variables are known to be correct after years of no known issues or
> >> bugs related
> >> to their current values.
> >>
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >>  drivers/net/ethernet/intel/igb/e1000_i210.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Thank Nick, I will add your patch to my queue.
> > 
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

[Bjørn Mork]

Jeff Kirsher <jeffrey.t.kirsher@intel.com> writes:

> If you want to see Nick's patch, feel free to view his patch on
> my queue tree:
> https://git.kernel.org/cgit/linux/kernel/git/jkirsher/queue.git/

which said:
  - s32 i = 0, timeout = 200; /* FIXME: find real value to use here */
  + s32 i = 0, timeout = 200;


Comments like the one deleted by that patch do have some value in my
opinion: They document that the constant is chosen arbitrarily and is
not taken from some datasheet.

Not a big deal, but leaving such comments, even if the value never ever
changes, could save someone from trying to figure out where the heck you
got that constant.  And there's noone actually misinterpreting this
comment as "something needs to be fixed here", is there? So the cost of
leaving the comment is exactly zero.

Just my .02 €.  I'm not going to tell you how to maintain your driver,
of course.  At least I try not to :-)


Bjørn

Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function,gb_acquire_swfw_sync_i210

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Mon, 2015-02-09 at 10:29 +0100, Bjørn Mork wrote:
> Jeff Kirsher <jeffrey.t.kirsher@intel.com> writes:
> 
> > If you want to see Nick's patch, feel free to view his patch on
> > my queue tree:
> > https://git.kernel.org/cgit/linux/kernel/git/jkirsher/queue.git/
> 
> which said:
>   - s32 i = 0, timeout = 200; /* FIXME: find real value to use here */
>   + s32 i = 0, timeout = 200;
> 
> 
> Comments like the one deleted by that patch do have some value in my
> opinion: They document that the constant is chosen arbitrarily and is
> not taken from some datasheet.
> 
> Not a big deal, but leaving such comments, even if the value never
> ever
> changes, could save someone from trying to figure out where the heck
> you
> got that constant.  And there's noone actually misinterpreting this
> comment as "something needs to be fixed here", is there? So the cost
> of
> leaving the comment is exactly zero.
> 
> Just my .02 €.  I'm not going to tell you how to maintain your driver,
> of course.  At least I try not to :-)

I see your point and I will have Carolyn take look to see if the comment
should rather be changed to see if we can describe why the value and
remove the FIXME at the same time.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [E1000-devel] [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function, gb_acquire_swfw_sync_i210

[Hisashi T Fujinaka]

On Mon, 9 Feb 2015, Bj?rn Mork wrote:

> Jeff Kirsher <jeffrey.t.kirsher@intel.com> writes:
>
>> If you want to see Nick's patch, feel free to view his patch on
>> my queue tree:
>> https://git.kernel.org/cgit/linux/kernel/git/jkirsher/queue.git/
>
> which said:
>  - s32 i = 0, timeout = 200; /* FIXME: find real value to use here */
>  + s32 i = 0, timeout = 200;
>
>
> Comments like the one deleted by that patch do have some value in my
> opinion: They document that the constant is chosen arbitrarily and is
> not taken from some datasheet.
>
> Not a big deal, but leaving such comments, even if the value never ever
> changes, could save someone from trying to figure out where the heck you
> got that constant.  And there's noone actually misinterpreting this
> comment as "something needs to be fixed here", is there? So the cost of
> leaving the comment is exactly zero.
>
> Just my .02 ?.  I'm not going to tell you how to maintain your driver,
> of course.  At least I try not to :-)

I think, for now, we should NAK this. It's a short comment and you don't
want me to replace it with a philosophical statement on how you could
tune the timeout. That's what the data sheet is for.

(Replying from my home account and not my work account -
todd.fujinaka@intel.com)