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

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

[Nicholas Krause]

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(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_i210.c b/drivers/net/ethernet/intel/igb/e1000_i210.c
index 65d9316..0c169e5 100644
--- a/drivers/net/ethernet/intel/igb/e1000_i210.c
+++ b/drivers/net/ethernet/intel/igb/e1000_i210.c
@@ -140,7 +140,7 @@ s32 igb_acquire_swfw_sync_i210(struct e1000_hw *hw, u16 mask)
 	u32 swmask = mask;
 	u32 fwmask = mask << 16;
 	s32 ret_val = 0;
-	s32 i = 0, timeout = 200; /* FIXME: find real value to use here */
+	s32 i = 0, timeout = 200;
 
 	while (i < timeout) {
 		if (igb_get_hw_semaphore_i210(hw)) {
-- 
2.1.0


------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Jeff Kirsher]

[-- Attachment #1.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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 441 bytes --]

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Jeff Kirsher]

[-- Attachment #1.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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 441 bytes --]

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[nick]

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

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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: [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)

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Nick Krause]

On Mon, Feb 9, 2015 at 12:03 PM, Hisashi T Fujinaka <htodd@twofifty.com> wrote:
> 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)
>
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> E1000-devel mailing list
> E1000-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
> To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired
That's fine, however if that is the case why not just explain the
reasons for these values in
another patch with a added comment rather then just removing the FIXME.
 Nick

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired