Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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!
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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.
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
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 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 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 Signed-off-by: Paul Menzel 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
Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
On Tue, 3 Nov 2020 00:13:07 +0100 Paul Menzel wrote: > From: Jeffrey Townsend > > 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 Jefferey will need to provide a sign-off as the author. > Cc: John W Linville > Signed-off-by: Paul Menzel
[PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence
From: Jeffrey Townsend
The ops field might no be defined, so add a check.
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
Cc: Jeffrey Townsend
Cc: John W Linville
Signed-off-by: Paul Menzel
---
drivers/net/ethernet/intel/igb/e1000_phy.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c
b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 4e0b4ba09a00..4151e55a6d2a 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -1107,11 +1107,13 @@ s32 igb_setup_copper_link(struct e1000_hw *hw)
/* PHY will be set to 10H, 10F, 100H or 100F
* depending on user settings.
*/
- hw_dbg("Forcing Speed and Duplex\n");
- ret_val = hw->phy.ops.force_speed_duplex(hw);
- if (ret_val) {
- hw_dbg("Error Forcing Speed and Duplex\n");
- goto out;
+ if (hw->phy.ops.force_speed_duplex) {
+ hw_dbg("Forcing Speed and Duplex\n");
+ ret_val = hw->phy.ops.force_speed_duplex(hw);
+ if (ret_val) {
+ hw_dbg("Error Forcing Speed and Duplex\n");
+ goto out;
+ }
}
}
--
2.29.1
