mbox series

[0/2] Upstream ONL patch for PHY BCM5461S

Message ID 20201102231307.13021-1-pmenzel@molgen.mpg.de
Headers show
Series Upstream ONL patch for PHY BCM5461S | expand

Message

Paul Menzel Nov. 2, 2020, 11:13 p.m. UTC
Dear Linux folks,


Looking a little bit at Open Network Linux, they carry some Linux
patches, but have not upstreamed them yet. This upstreams support for
the PHY BCM5461S. It’d be great, if you could help review it.


Kind regards,

Paul


Jeffrey Townsend (2):
  ethernet: igb: Support PHY BCM5461S
  ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

 drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 ++++-
 .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
 drivers/net/ethernet/intel/igb/e1000_phy.c    | 89 +++++++++++++++++--
 drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
 6 files changed, 118 insertions(+), 6 deletions(-)

Comments

Jakub Kicinski Nov. 3, 2020, 12:19 a.m. UTC | #1
On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

> 

> The ops field might no be defined, so add a check.


This change should be first, otherwise AFAIU if someone builds the
kernel in between the commits (e.g. for bisection) it will crash.

> The patch is taken from Open Network Linux (ONL), and it was added there

> as part of the patch

> 

>     packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

> 

> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part

> of this commit was already upstreamed in Linux commit eeb0149660 (igb:

> support BCM54616 PHY) in 2017.

> 

> I applied the forward-ported

> 

>     packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

> 

> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

> 

> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1

> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331


No need to put this in every commit message.

We preserve the cover letter in tree as a merge commit message, so
explaining things once in the cover letter is sufficient.

> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>


Jefferey will need to provide a sign-off as the author.

> Cc: John W Linville <linville@tuxdriver.com>

> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Paul Menzel Nov. 3, 2020, 7:35 a.m. UTC | #2
Dear Jakub,


Am 03.11.20 um 01:19 schrieb Jakub Kicinski:
> On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:

>> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

>>

>> The ops field might no be defined, so add a check.

> 

> This change should be first, otherwise AFAIU if someone builds the

> kernel in between the commits (e.g. for bisection) it will crash.


Patch `[PATCH 1/2] ethernet: igb: Support PHY BCM5461S` has

     phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;

so the ordering does not matter. I do not know, if Jeffrey can comment, 
but probably the check was just adding during development. Maybe an 
assert should be added instead?

>> The patch is taken from Open Network Linux (ONL), and it was added there

>> as part of the patch

>>

>>      packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

>>

>> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part

>> of this commit was already upstreamed in Linux commit eeb0149660 (igb:

>> support BCM54616 PHY) in 2017.

>>

>> I applied the forward-ported

>>

>>      packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

>>

>> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

>>

>> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1

>> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

> 

> No need to put this in every commit message.

> 

> We preserve the cover letter in tree as a merge commit message, so

> explaining things once in the cover letter is sufficient.


I remember, but still find it confusing. If I look at a commit with `git 
show …`, I normally do not think of also looking at a possible cover 
letter as not many subsystems/projects do it, and I assume a commit is 
self-contained.

Could you share your development process

>> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

> 

> Jefferey will need to provide a sign-off as the author.


According to *Developer's Certificate of Origin 1.1* [3], it’s my 
understanding, that it is *not* required. The items (a), (b), and (c) 
are connected by an *or*.

>         (b) The contribution is based upon previous work that, to the best

>             of my knowledge, is covered under an appropriate open source

>             license and I have the right under that license to submit that

>             work with modifications, whether created in whole or in part 

>             by me, under the same open source license (unless I am

>             permitted to submit under a different license), as indicated

>             in the file; or


>> Cc: John W Linville <linville@tuxdriver.com>

>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>



Kind regards,

Paul


[3]: 
https://www.kernel.org/doc/html/v5.9/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
Jakub Kicinski Nov. 3, 2020, 6:39 p.m. UTC | #3
On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
> According to *Developer's Certificate of Origin 1.1* [3], it’s my 

> understanding, that it is *not* required. The items (a), (b), and (c) 

> are connected by an *or*.

> 

> >         (b) The contribution is based upon previous work that, to the best

> >             of my knowledge, is covered under an appropriate open source

> >             license and I have the right under that license to submit that

> >             work with modifications, whether created in whole or in part 

> >             by me, under the same open source license (unless I am

> >             permitted to submit under a different license), as indicated

> >             in the file; or  


Ack, but then you need to put yourself as the author, because it's
you certifying that the code falls under (b).

At least that's my understanding.
Paul Menzel Jan. 5, 2021, 5:16 p.m. UTC | #4
Dear Jakub, dear Greg,


Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:

>> According to *Developer's Certificate of Origin 1.1* [3], it’s my

>> understanding, that it is *not* required. The items (a), (b), and (c)

>> are connected by an *or*.

>>

>>>          (b) The contribution is based upon previous work that, to the best

>>>              of my knowledge, is covered under an appropriate open source

>>>              license and I have the right under that license to submit that

>>>              work with modifications, whether created in whole or in part

>>>              by me, under the same open source license (unless I am

>>>              permitted to submit under a different license), as indicated

>>>              in the file; or

> 

> Ack, but then you need to put yourself as the author, because it's

> you certifying that the code falls under (b).

> 

> At least that's my understanding.


Greg, can you please clarify, if it’s fine, if I upstream a patch 
authored by somebody else and distributed under the GPLv2? I put them as 
the author and signed it off.

(In this case the change, adding an if condition, is also trivial.)


Kind regards,

Paul
gregkh@linuxfoundation.org Jan. 5, 2021, 5:25 p.m. UTC | #5
On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:
> Dear Jakub, dear Greg,

> 

> 

> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:

> > On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:

> > > According to *Developer's Certificate of Origin 1.1* [3], it’s my

> > > understanding, that it is *not* required. The items (a), (b), and (c)

> > > are connected by an *or*.

> > > 

> > > >          (b) The contribution is based upon previous work that, to the best

> > > >              of my knowledge, is covered under an appropriate open source

> > > >              license and I have the right under that license to submit that

> > > >              work with modifications, whether created in whole or in part

> > > >              by me, under the same open source license (unless I am

> > > >              permitted to submit under a different license), as indicated

> > > >              in the file; or

> > 

> > Ack, but then you need to put yourself as the author, because it's

> > you certifying that the code falls under (b).

> > 

> > At least that's my understanding.

> 

> Greg, can you please clarify, if it’s fine, if I upstream a patch authored

> by somebody else and distributed under the GPLv2? I put them as the author

> and signed it off.


You can't add someone else's signed-off-by, but you can add your own and
keep them as the author, has happened lots of time in the past.

Or, you can make the From: line be from you if the original author
doesn't want their name/email in the changelog, we've done that as well,
both are fine.

thanks,

greg k-h
Paul Menzel Jan. 19, 2021, 6:55 a.m. UTC | #6
Dear Jakub, dear Greg,


Am 05.01.21 um 18:25 schrieb Greg KH:
> On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:


>> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:

>>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:

>>>> According to *Developer's Certificate of Origin 1.1* [3], it’s my

>>>> understanding, that it is *not* required. The items (a), (b), and (c)

>>>> are connected by an *or*.

>>>>

>>>>>           (b) The contribution is based upon previous work that, to the best

>>>>>               of my knowledge, is covered under an appropriate open source

>>>>>               license and I have the right under that license to submit that

>>>>>               work with modifications, whether created in whole or in part

>>>>>               by me, under the same open source license (unless I am

>>>>>               permitted to submit under a different license), as indicated

>>>>>               in the file; or

>>>

>>> Ack, but then you need to put yourself as the author, because it's

>>> you certifying that the code falls under (b).

>>>

>>> At least that's my understanding.

>>

>> Greg, can you please clarify, if it’s fine, if I upstream a patch authored

>> by somebody else and distributed under the GPLv2? I put them as the author

>> and signed it off.

> 

> You can't add someone else's signed-off-by, but you can add your own and

> keep them as the author, has happened lots of time in the past.

> 

> Or, you can make the From: line be from you if the original author

> doesn't want their name/email in the changelog, we've done that as well,

> both are fine.


Greg, thank you for the clarification.

Jakub, with that out of the way, can you please take patch 2/2?


Kind regards,

Paul
Jakub Kicinski Jan. 19, 2021, 5:05 p.m. UTC | #7
On Tue, 19 Jan 2021 07:55:19 +0100 Paul Menzel wrote:
> Am 05.01.21 um 18:25 schrieb Greg KH:

> > On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:  

> >> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:  

> >>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:  

> >>>> According to *Developer's Certificate of Origin 1.1* [3], it’s my

> >>>> understanding, that it is *not* required. The items (a), (b), and (c)

> >>>> are connected by an *or*.

> >>>>  

> >>>>>           (b) The contribution is based upon previous work that, to the best

> >>>>>               of my knowledge, is covered under an appropriate open source

> >>>>>               license and I have the right under that license to submit that

> >>>>>               work with modifications, whether created in whole or in part

> >>>>>               by me, under the same open source license (unless I am

> >>>>>               permitted to submit under a different license), as indicated

> >>>>>               in the file; or  

> >>>

> >>> Ack, but then you need to put yourself as the author, because it's

> >>> you certifying that the code falls under (b).

> >>>

> >>> At least that's my understanding.  

> >>

> >> Greg, can you please clarify, if it’s fine, if I upstream a patch authored

> >> by somebody else and distributed under the GPLv2? I put them as the author

> >> and signed it off.  

> > 

> > You can't add someone else's signed-off-by, but you can add your own and

> > keep them as the author, has happened lots of time in the past.

> > 

> > Or, you can make the From: line be from you if the original author

> > doesn't want their name/email in the changelog, we've done that as well,

> > both are fine.  

> 

> Greg, thank you for the clarification.

> 

> Jakub, with that out of the way, can you please take patch 2/2?


Please repost the patches, if you know how please add a lore link to
this posting, thanks!