2015-02-09 01:21:33

by Jeff Kirsher

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

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 <[email protected]>
> ---
> 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.


Attachments:
signature.asc (819.00 B)
This is a digitally signed message part

2015-02-09 01:26:12

by Jeff Kirsher

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

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 <[email protected]>
> > ---
> > 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 ..."


Attachments:
signature.asc (819.00 B)
This is a digitally signed message part

2015-02-09 07:17:20

by Andrej Manduch

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

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 <[email protected]>
>> ---
>> 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.

2015-02-09 08:32:06

by Jeff Kirsher

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

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 <[email protected]>
> >> ---
> >> 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.
> >
>



Attachments:
signature.asc (819.00 B)
This is a digitally signed message part

2015-02-09 09:30:04

by Bjørn Mork

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

Jeff Kirsher <[email protected]> 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

2015-02-09 09:35:35

by Jeff Kirsher

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

On Mon, 2015-02-09 at 10:29 +0100, Bjørn Mork wrote:
> Jeff Kirsher <[email protected]> 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.


Attachments:
signature.asc (819.00 B)
This is a digitally signed message part

2015-02-09 17:12:59

by Hisashi T Fujinaka

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

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

> Jeff Kirsher <[email protected]> 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 -
[email protected])