Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1220614 > unrolled thread
| Started by | Alexey Brodkin <Alexey.Brodkin@synopsys.com> |
|---|---|
| First post | 2015-09-08 11:10 +0200 |
| Last post | 2015-09-09 16:50 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v3] stmmac: fix check for phydev being open Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2015-09-08 11:10 +0200
Re: [PATCH v3] stmmac: fix check for phydev being open Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-09-08 13:30 +0200
Re: [PATCH v3] stmmac: fix check for phydev being open Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2015-09-08 14:50 +0200
Re: [PATCH v3] stmmac: fix check for phydev being open Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-09-08 22:00 +0200
Re: [PATCH v3] stmmac: fix check for phydev being open Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2015-09-09 16:50 +0200
| From | Alexey Brodkin <Alexey.Brodkin@synopsys.com> |
|---|---|
| Date | 2015-09-08 11:10 +0200 |
| Subject | [PATCH v3] stmmac: fix check for phydev being open |
| Message-ID | <q6n4C-8cb-7@gated-at.bofh.it> |
Current check of phydev with IS_ERR(phydev) may make not much sense
because of_phy_connect() returns NULL on failure instead of error value.
Still for checking result of phy_connect() IS_ERR() makes perfect sense.
So let's use combined check IS_ERR_OR_NULL() that covers both cases.
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
---
Changes compared to v2:
* Updated commit message with mention of of_phy_connect() instead of
of_phy_attach().
* Return ENODEV in case of of_phy_connect() failure
Changes compared to v1:
* Use IS_ERR_OR_NULL() instead of discrete checks for null and err
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 864b476..e2c9c86 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -837,9 +837,12 @@ static int stmmac_init_phy(struct net_device *dev)
interface);
}
- if (IS_ERR(phydev)) {
+ if (IS_ERR_OR_NULL(phydev)) {
pr_err("%s: Could not attach to PHY\n", dev->name);
- return PTR_ERR(phydev);
+ if (!phydev)
+ return -ENODEV;
+ else
+ return PTR_ERR(phydev);
}
/* Stop Advertising 1000BASE Capability if interface is not GMII */
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> |
|---|---|
| Date | 2015-09-08 13:30 +0200 |
| Message-ID | <q6pg6-2No-21@gated-at.bofh.it> |
| In reply to | #1220614 |
Hello.
On 9/8/2015 11:43 AM, Alexey Brodkin wrote:
> Current check of phydev with IS_ERR(phydev) may make not much sense
> because of_phy_connect() returns NULL on failure instead of error value.
>
> Still for checking result of phy_connect() IS_ERR() makes perfect sense.
>
> So let's use combined check IS_ERR_OR_NULL() that covers both cases.
>
> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Cc: David Miller <davem@davemloft.net>
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>
> Changes compared to v2:
> * Updated commit message with mention of of_phy_connect() instead of
> of_phy_attach().
> * Return ENODEV in case of of_phy_connect() failure
>
> Changes compared to v1:
> * Use IS_ERR_OR_NULL() instead of discrete checks for null and err
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 864b476..e2c9c86 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -837,9 +837,12 @@ static int stmmac_init_phy(struct net_device *dev)
> interface);
> }
>
> - if (IS_ERR(phydev)) {
> + if (IS_ERR_OR_NULL(phydev)) {
> pr_err("%s: Could not attach to PHY\n", dev->name);
> - return PTR_ERR(phydev);
> + if (!phydev)
> + return -ENODEV;
> + else
> + return PTR_ERR(phydev);
Don't need *else* after *return* and scripts/checkpatch.pl should have
complained about that.
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Alexey Brodkin <Alexey.Brodkin@synopsys.com> |
|---|---|
| Date | 2015-09-08 14:50 +0200 |
| Message-ID | <q6qvv-4v7-9@gated-at.bofh.it> |
| In reply to | #1220711 |
Hi Sergei, On Tue, 2015-09-08 at 14:20 +-0300, Sergei Shtylyov wrote: +AD4- Hello. +AD4- +AD4- On 9/8/2015 11:43 AM, Alexey Brodkin wrote: +AD4- +AD4- +AD4- Current check of phydev with IS+AF8-ERR(phydev) may make not much sense +AD4- +AD4- because of+AF8-phy+AF8-connect() returns NULL on failure instead of error value. +AD4- +AD4- +AD4- +AD4- Still for checking result of phy+AF8-connect() IS+AF8-ERR() makes perfect sense. +AD4- +AD4- +AD4- +AD4- So let's use combined check IS+AF8-ERR+AF8-OR+AF8-NULL() that covers both cases. +AD4- +AD4- +AD4- +AD4- Cc: Sergei Shtylyov +ADw-sergei.shtylyov+AEA-cogentembedded.com+AD4- +AD4- +AD4- Cc: Giuseppe Cavallaro +ADw-peppe.cavallaro+AEA-st.com+AD4- +AD4- +AD4- Cc: linux-kernel+AEA-vger.kernel.org +AD4- +AD4- Cc: stable+AEA-vger.kernel.org +AD4- +AD4- Cc: David Miller +ADw-davem+AEA-davemloft.net+AD4- +AD4- +AD4- Signed-off-by: Alexey Brodkin +ADw-abrodkin+AEA-synopsys.com+AD4- +AD4- +AD4- --- +AD4- +AD4- +AD4- +AD4- Changes compared to v2: +AD4- +AD4- +ACo- Updated commit message with mention of of+AF8-phy+AF8-connect() instead of +AD4- +AD4- of+AF8-phy+AF8-attach(). +AD4- +AD4- +ACo- Return ENODEV in case of of+AF8-phy+AF8-connect() failure +AD4- +AD4- +AD4- +AD4- Changes compared to v1: +AD4- +AD4- +ACo- Use IS+AF8-ERR+AF8-OR+AF8-NULL() instead of discrete checks for null and err +AD4- +AD4- +AD4- +AD4- drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AHw- 7 +-+-+-+-+--- +AD4- +AD4- 1 file changed, 5 insertions(+-), 2 deletions(-) +AD4- +AD4- +AD4- +AD4- diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- index 864b476..e2c9c86 100644 +AD4- +AD4- --- a/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +-+-+- b/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +AEAAQA- -837,9 +-837,12 +AEAAQA- static int stmmac+AF8-init+AF8-phy(struct net+AF8-device +ACo-dev) +AD4- +AD4- interface)+ADs- +AD4- +AD4- +AH0- +AD4- +AD4- +AD4- +AD4- - if (IS+AF8-ERR(phydev)) +AHs- +AD4- +AD4- +- if (IS+AF8-ERR+AF8-OR+AF8-NULL(phydev)) +AHs- +AD4- +AD4- pr+AF8-err(+ACIAJQ-s: Could not attach to PHY+AFw-n+ACI-, dev-+AD4-name)+ADs- +AD4- +AD4- - return PTR+AF8-ERR(phydev)+ADs- +AD4- +AD4- +- if (+ACE-phydev) +AD4- +AD4- +- return -ENODEV+ADs- +AD4- +AD4- +- else +AD4- +AD4- +- return PTR+AF8-ERR(phydev)+ADs- +AD4- +AD4- Don't need +ACo-else+ACo- after +ACo-return+ACo- and scripts/checkpatch.pl should have +AD4- complained about that. ./scripts/checkpatch.pl 0001-stmmac-fix-check-for-phydev-being-open.patch total: 0 errors, 0 warnings, 0 checks, 14 lines checked -Alexey-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> |
|---|---|
| Date | 2015-09-08 22:00 +0200 |
| Message-ID | <q6xdE-5Kp-11@gated-at.bofh.it> |
| In reply to | #1220743 |
Hello.
On 09/08/2015 03:46 PM, Alexey Brodkin wrote:
>>> Current check of phydev with IS_ERR(phydev) may make not much sense
>>> because of_phy_connect() returns NULL on failure instead of error value.
>>>
>>> Still for checking result of phy_connect() IS_ERR() makes perfect sense.
>>>
>>> So let's use combined check IS_ERR_OR_NULL() that covers both cases.
>>>
>>> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: stable@vger.kernel.org
>>> Cc: David Miller <davem@davemloft.net>
>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>> ---
>>>
>>> Changes compared to v2:
>>> * Updated commit message with mention of of_phy_connect() instead of
>>> of_phy_attach().
>>> * Return ENODEV in case of of_phy_connect() failure
>>>
>>> Changes compared to v1:
>>> * Use IS_ERR_OR_NULL() instead of discrete checks for null and err
>>>
>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index 864b476..e2c9c86 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -837,9 +837,12 @@ static int stmmac_init_phy(struct net_device *dev)
>>> interface);
>>> }
>>>
>>> - if (IS_ERR(phydev)) {
>>> + if (IS_ERR_OR_NULL(phydev)) {
>>> pr_err("%s: Could not attach to PHY\n", dev->name);
>>> - return PTR_ERR(phydev);
>>> + if (!phydev)
>>> + return -ENODEV;
>>> + else
>>> + return PTR_ERR(phydev);
>>
>> Don't need *else* after *return* and scripts/checkpatch.pl should have
>> complained about that.
>
> ./scripts/checkpatch.pl 0001-stmmac-fix-check-for-phydev-being-open.patch
> total: 0 errors, 0 warnings, 0 checks, 14 lines checked
Hm... I bet I saw such warning from checkpatch.pl recently (it was a false
positive though, so maybe the check was removed recently, not sure). Your
patch is clean indeed, however my comment is still valid.
> -Alexey
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Alexey Brodkin <Alexey.Brodkin@synopsys.com> |
|---|---|
| Date | 2015-09-09 16:50 +0200 |
| Message-ID | <q6ORb-62m-17@gated-at.bofh.it> |
| In reply to | #1221058 |
Hi Sergei, On Tue, 2015-09-08 at 22:53 +-0300, Sergei Shtylyov wrote: +AD4- Hello. +AD4- +AD4- On 09/08/2015 03:46 PM, Alexey Brodkin wrote: +AD4- +AD4- +AD4- +AD4- +AD4- Current check of phydev with IS+AF8-ERR(phydev) may make not much sense +AD4- +AD4- +AD4- +AD4- because of+AF8-phy+AF8-connect() returns NULL on failure instead of error value. +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Still for checking result of phy+AF8-connect() IS+AF8-ERR() makes perfect sense. +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- So let's use combined check IS+AF8-ERR+AF8-OR+AF8-NULL() that covers both cases. +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Cc: Sergei Shtylyov +ADw-sergei.shtylyov+AEA-cogentembedded.com+AD4- +AD4- +AD4- +AD4- +AD4- Cc: Giuseppe Cavallaro +ADw-peppe.cavallaro+AEA-st.com+AD4- +AD4- +AD4- +AD4- +AD4- Cc: linux-kernel+AEA-vger.kernel.org +AD4- +AD4- +AD4- +AD4- Cc: stable+AEA-vger.kernel.org +AD4- +AD4- +AD4- +AD4- Cc: David Miller +ADw-davem+AEA-davemloft.net+AD4- +AD4- +AD4- +AD4- +AD4- Signed-off-by: Alexey Brodkin +ADw-abrodkin+AEA-synopsys.com+AD4- +AD4- +AD4- +AD4- +AD4- --- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Changes compared to v2: +AD4- +AD4- +AD4- +AD4- +ACo- Updated commit message with mention of of+AF8-phy+AF8-connect() instead of +AD4- +AD4- +AD4- +AD4- of+AF8-phy+AF8-attach(). +AD4- +AD4- +AD4- +AD4- +ACo- Return ENODEV in case of of+AF8-phy+AF8-connect() failure +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Changes compared to v1: +AD4- +AD4- +AD4- +AD4- +ACo- Use IS+AF8-ERR+AF8-OR+AF8-NULL() instead of discrete checks for null and err +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AHw- 7 +-+-+-+-+--- +AD4- +AD4- +AD4- +AD4- 1 file changed, 5 insertions(+-), 2 deletions(-) +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +AD4- +AD4- b/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +AD4- +AD4- index 864b476..e2c9c86 100644 +AD4- +AD4- +AD4- +AD4- --- a/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +AD4- +AD4- +-+-+- b/drivers/net/ethernet/stmicro/stmmac/stmmac+AF8-main.c +AD4- +AD4- +AD4- +AD4- +AEAAQA- -837,9 +-837,12 +AEAAQA- static int stmmac+AF8-init+AF8-phy(struct net+AF8-device +ACo-dev) +AD4- +AD4- +AD4- +AD4- interface)+ADs- +AD4- +AD4- +AD4- +AD4- +AH0- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- - if (IS+AF8-ERR(phydev)) +AHs- +AD4- +AD4- +AD4- +AD4- +- if (IS+AF8-ERR+AF8-OR+AF8-NULL(phydev)) +AHs- +AD4- +AD4- +AD4- +AD4- pr+AF8-err(+ACIAJQ-s: Could not attach to PHY+AFw-n+ACI-, dev-+AD4-name)+ADs- +AD4- +AD4- +AD4- +AD4- - return PTR+AF8-ERR(phydev)+ADs- +AD4- +AD4- +AD4- +AD4- +- if (+ACE-phydev) +AD4- +AD4- +AD4- +AD4- +- return -ENODEV+ADs- +AD4- +AD4- +AD4- +AD4- +- else +AD4- +AD4- +AD4- +AD4- +- return PTR+AF8-ERR(phydev)+ADs- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Don't need +ACo-else+ACo- after +ACo-return+ACo- and scripts/checkpatch.pl should have +AD4- +AD4- +AD4- complained about that. +AD4- +AD4- +AD4- +AD4- ./scripts/checkpatch.pl 0001-stmmac-fix-check-for-phydev-being-open.patch +AD4- +AD4- total: 0 errors, 0 warnings, 0 checks, 14 lines checked +AD4- +AD4- Hm... I bet I saw such warning from checkpatch.pl recently (it was a false +AD4- positive though, so maybe the check was removed recently, not sure). Your +AD4- patch is clean indeed, however my comment is still valid. Ok, let me send another respin then. -Alexey-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web