* [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
@ 2007-05-10 17:29 Matvejchikov Ilya
2007-05-10 17:56 ` Randy Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Matvejchikov Ilya @ 2007-05-10 17:29 UTC (permalink / raw)
To: netdev; +Cc: jeff
Good Day!
The command 'brctl addbr br0 eth0' brings the kernel oops if the eth0
has PHY, but the phydev is NULL (for ex., ifconfig eth0 0.0.0.0 was
not issued firstly)
Call Trace:
[C02CFBD0] [7FFFFFFF] 0x7fffffff (unreliable)
[C02CFBE0] [C0109634] dev_ethtool+0x1b0/0xfac
[C02CFCD0] [C0155EF0] port_cost+0x5c/0x130
[C02CFD40] [C01561FC] br_add_if+0x17c/0x308
[C02CFD70] [C01569E0] add_del_if+0x70/0xa0
[C02CFD90] [C0156A58] br_dev_ioctl+0x48/0x9fc
[C02CFE10] [C0106FCC] dev_ifsioc+0x144/0x3ac
[C02CFE30] [C01080D8] dev_ioctl+0x3b8/0x4c8
[C02CFEB0] [C00F9BC0] sock_ioctl+0x78/0x260
[C02CFED0] [C006889C] do_ioctl+0x38/0x84
[C02CFEE0] [C006896C] vfs_ioctl+0x84/0x3d8
[C02CFF10] [C0068D00] sys_ioctl+0x40/0x74
[C02CFF40] [C000EF48] ret_from_syscall+0x0/0x38
The following patch fixes the problem.
Signed-off-by: Matvejchikov Ilya <matvejchikov <at> gmail.com>
---
diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
linux-2.6.21/drivers/net/phy/phy.c
--- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
07:08:32.000000000 +0400
+++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04 08:22:01.000000000 +0400
@@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
*/
int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
{
+ if (unlikely(NULL == phydev))
+ return -EINVAL;
+
if (cmd->phy_address != phydev->addr)
return -EINVAL;
@@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
{
+ if (unlikely(NULL == phydev))
+ return -EINVAL;
+
cmd->supported = phydev->supported;
cmd->advertising = phydev->advertising;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-10 17:29 [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL Matvejchikov Ilya
@ 2007-05-10 17:56 ` Randy Dunlap
2007-05-10 18:19 ` Jeff Garzik
0 siblings, 1 reply; 9+ messages in thread
From: Randy Dunlap @ 2007-05-10 17:56 UTC (permalink / raw)
To: matvejchikov; +Cc: netdev, jeff
On Thu, 10 May 2007 21:29:51 +0400 Matvejchikov Ilya wrote:
> Good Day!
>
> The command 'brctl addbr br0 eth0' brings the kernel oops if the eth0
> has PHY, but the phydev is NULL (for ex., ifconfig eth0 0.0.0.0 was
> not issued firstly)
>
> Call Trace:
> [C02CFBD0] [7FFFFFFF] 0x7fffffff (unreliable)
> [C02CFBE0] [C0109634] dev_ethtool+0x1b0/0xfac
> [C02CFCD0] [C0155EF0] port_cost+0x5c/0x130
> [C02CFD40] [C01561FC] br_add_if+0x17c/0x308
> [C02CFD70] [C01569E0] add_del_if+0x70/0xa0
> [C02CFD90] [C0156A58] br_dev_ioctl+0x48/0x9fc
> [C02CFE10] [C0106FCC] dev_ifsioc+0x144/0x3ac
> [C02CFE30] [C01080D8] dev_ioctl+0x3b8/0x4c8
> [C02CFEB0] [C00F9BC0] sock_ioctl+0x78/0x260
> [C02CFED0] [C006889C] do_ioctl+0x38/0x84
> [C02CFEE0] [C006896C] vfs_ioctl+0x84/0x3d8
> [C02CFF10] [C0068D00] sys_ioctl+0x40/0x74
> [C02CFF40] [C000EF48] ret_from_syscall+0x0/0x38
>
> The following patch fixes the problem.
>
> Signed-off-by: Matvejchikov Ilya <matvejchikov <at> gmail.com>
Don't obfuscate the email address.
> ---
>
> diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
> linux-2.6.21/drivers/net/phy/phy.c
> --- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
> 07:08:32.000000000 +0400
> +++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04 08:22:01.000000000 +0400
> @@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
> */
> int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
> {
> + if (unlikely(NULL == phydev))
> + return -EINVAL;
Use tabs instead of spaces for indentation.
and I hope that Jeff prefers (as some others do):
if (unlikely(phydev == NULL))
or even
if (unlikely(!phydev))
> +
> if (cmd->phy_address != phydev->addr)
> return -EINVAL;
>
> @@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
>
> int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
> {
> + if (unlikely(NULL == phydev))
> + return -EINVAL;
> +
> cmd->supported = phydev->supported;
>
> cmd->advertising = phydev->advertising;
> -
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-10 17:56 ` Randy Dunlap
@ 2007-05-10 18:19 ` Jeff Garzik
2007-05-11 10:58 ` Matvejchikov Ilya
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Garzik @ 2007-05-10 18:19 UTC (permalink / raw)
To: Randy Dunlap; +Cc: matvejchikov, netdev
Randy Dunlap wrote:
> and I hope that Jeff prefers (as some others do):
>
> if (unlikely(phydev == NULL))
>
> or even
>
> if (unlikely(!phydev))
That is indeed my preference :)
Jeff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-10 18:19 ` Jeff Garzik
@ 2007-05-11 10:58 ` Matvejchikov Ilya
2007-05-11 13:18 ` Kumar Gala
2007-05-21 19:24 ` Andy Fleming
0 siblings, 2 replies; 9+ messages in thread
From: Matvejchikov Ilya @ 2007-05-11 10:58 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Randy Dunlap, netdev
Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
---
diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
linux-2.6.21/drivers/net/phy/phy.c
--- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26 07:08:32.000000000 +0400
+++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04 08:22:01.000000000 +0400
@@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
*/
int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
{
+ if (unlikely(!phydev))
+ return -EINVAL;
+
if (cmd->phy_address != phydev->addr)
return -EINVAL;
@@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
{
+ if (unlikely(!phydev))
+ return -EINVAL;
+
cmd->supported = phydev->supported;
cmd->advertising = phydev->advertising;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-11 10:58 ` Matvejchikov Ilya
@ 2007-05-11 13:18 ` Kumar Gala
2007-05-11 13:49 ` Matvejchikov Ilya
2007-05-21 19:24 ` Andy Fleming
1 sibling, 1 reply; 9+ messages in thread
From: Kumar Gala @ 2007-05-11 13:18 UTC (permalink / raw)
To: matvejchikov; +Cc: Jeff Garzik, Randy Dunlap, netdev
On May 11, 2007, at 5:58 AM, Matvejchikov Ilya wrote:
> Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
> ---
>
> diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
> linux-2.6.21/drivers/net/phy/phy.c
> --- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
> 07:08:32.000000000 +0400
> +++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04
> 08:22:01.000000000 +0400
> @@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
> */
> int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd
> *cmd)
> {
> + if (unlikely(!phydev))
> + return -EINVAL;
Should this be -ENODEV?
> +
> if (cmd->phy_address != phydev->addr)
> return -EINVAL;
>
> @@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
>
> int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd
> *cmd)
> {
> + if (unlikely(!phydev))
> + return -EINVAL;
same question.
> +
> cmd->supported = phydev->supported;
>
> cmd->advertising = phydev->advertising;
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-11 13:18 ` Kumar Gala
@ 2007-05-11 13:49 ` Matvejchikov Ilya
2007-05-11 14:44 ` Kumar Gala
0 siblings, 1 reply; 9+ messages in thread
From: Matvejchikov Ilya @ 2007-05-11 13:49 UTC (permalink / raw)
To: Kumar Gala; +Cc: Jeff Garzik, Randy Dunlap, netdev
2007/5/11, Kumar Gala <galak@kernel.crashing.org>:
>
> On May 11, 2007, at 5:58 AM, Matvejchikov Ilya wrote:
>
> > Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
> > ---
> >
> > diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
> > linux-2.6.21/drivers/net/phy/phy.c
> > --- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
> > 07:08:32.000000000 +0400
> > +++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04
> > 08:22:01.000000000 +0400
> > @@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
> > */
> > int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd
> > *cmd)
> > {
> > + if (unlikely(!phydev))
> > + return -EINVAL;
>
> Should this be -ENODEV?
If we get the ENODEV error, we are suppose that there is no device at
all. PHY device may not be connected to the NET device. But if it
exists we can't say that there is NODEV...
I think it should be -EINVAL.
> > +
> > if (cmd->phy_address != phydev->addr)
> > return -EINVAL;
> >
> > @@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
> >
> > int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd
> > *cmd)
> > {
> > + if (unlikely(!phydev))
> > + return -EINVAL;
>
> same question.
The same answer :)
> > +
> > cmd->supported = phydev->supported;
> >
> > cmd->advertising = phydev->advertising;
> > -
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-11 13:49 ` Matvejchikov Ilya
@ 2007-05-11 14:44 ` Kumar Gala
2007-05-14 19:32 ` Matvejchikov Ilya
0 siblings, 1 reply; 9+ messages in thread
From: Kumar Gala @ 2007-05-11 14:44 UTC (permalink / raw)
To: matvejchikov; +Cc: Jeff Garzik, Randy Dunlap, netdev
On May 11, 2007, at 8:49 AM, Matvejchikov Ilya wrote:
> 2007/5/11, Kumar Gala <galak@kernel.crashing.org>:
>>
>> On May 11, 2007, at 5:58 AM, Matvejchikov Ilya wrote:
>>
>> > Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
>> > ---
>> >
>> > diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
>> > linux-2.6.21/drivers/net/phy/phy.c
>> > --- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
>> > 07:08:32.000000000 +0400
>> > +++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04
>> > 08:22:01.000000000 +0400
>> > @@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
>> > */
>> > int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd
>> > *cmd)
>> > {
>> > + if (unlikely(!phydev))
>> > + return -EINVAL;
>>
>> Should this be -ENODEV?
>
> If we get the ENODEV error, we are suppose that there is no device at
> all. PHY device may not be connected to the NET device. But if it
> exists we can't say that there is NODEV...
>
> I think it should be -EINVAL.
Hmm, if the phy isn't connected to the NET device doesn't that mean
we don't have a PHY device (from the point of view of the netdevice),
thus NODEV?
Whatever we should cleanup the current users of phy_ethtool_sset/
phy_ethtool_gset.
>> > +
>> > if (cmd->phy_address != phydev->addr)
>> > return -EINVAL;
>> >
>> > @@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
>> >
>> > int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd
>> > *cmd)
>> > {
>> > + if (unlikely(!phydev))
>> > + return -EINVAL;
>>
>> same question.
>
> The same answer :)
>
>> > +
>> > cmd->supported = phydev->supported;
>> >
>> > cmd->advertising = phydev->advertising;
>> > -
>> > To unsubscribe from this list: send the line "unsubscribe
>> netdev" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-11 14:44 ` Kumar Gala
@ 2007-05-14 19:32 ` Matvejchikov Ilya
0 siblings, 0 replies; 9+ messages in thread
From: Matvejchikov Ilya @ 2007-05-14 19:32 UTC (permalink / raw)
To: Kumar Gala; +Cc: Jeff Garzik, Randy Dunlap, netdev
2007/5/11, Kumar Gala <galak@kernel.crashing.org>:
>
> On May 11, 2007, at 8:49 AM, Matvejchikov Ilya wrote:
>
> > 2007/5/11, Kumar Gala <galak@kernel.crashing.org>:
> >>
> >> On May 11, 2007, at 5:58 AM, Matvejchikov Ilya wrote:
> >>
> >> > Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
> >> > ---
> >> >
> >> > diff -purN linux-2.6.21-clean/drivers/net/phy/phy.c
> >> > linux-2.6.21/drivers/net/phy/phy.c
> >> > --- linux-2.6.21-clean/drivers/net/phy/phy.c 2007-04-26
> >> > 07:08:32.000000000 +0400
> >> > +++ linux-2.6.21/drivers/net/phy/phy.c 2007-05-04
> >> > 08:22:01.000000000 +0400
> >> > @@ -245,6 +245,9 @@ EXPORT_SYMBOL(phy_sanitize_settings);
> >> > */
> >> > int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd
> >> > *cmd)
> >> > {
> >> > + if (unlikely(!phydev))
> >> > + return -EINVAL;
> >>
> >> Should this be -ENODEV?
> >
> > If we get the ENODEV error, we are suppose that there is no device at
> > all. PHY device may not be connected to the NET device. But if it
> > exists we can't say that there is NODEV...
> >
> > I think it should be -EINVAL.
>
> Hmm, if the phy isn't connected to the NET device doesn't that mean
> we don't have a PHY device (from the point of view of the netdevice),
> thus NODEV?
It will hardly be understandable for the user...
>
> Whatever we should cleanup the current users of phy_ethtool_sset/
> phy_ethtool_gset.
>
Right you are.
> >> > +
> >> > if (cmd->phy_address != phydev->addr)
> >> > return -EINVAL;
> >> >
> >> > @@ -289,6 +292,9 @@ EXPORT_SYMBOL(phy_ethtool_sset);
> >> >
> >> > int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd
> >> > *cmd)
> >> > {
> >> > + if (unlikely(!phydev))
> >> > + return -EINVAL;
> >>
> >> same question.
> >
> > The same answer :)
> >
> >> > +
> >> > cmd->supported = phydev->supported;
> >> >
> >> > cmd->advertising = phydev->advertising;
> >> > -
> >> > To unsubscribe from this list: send the line "unsubscribe
> >> netdev" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL
2007-05-11 10:58 ` Matvejchikov Ilya
2007-05-11 13:18 ` Kumar Gala
@ 2007-05-21 19:24 ` Andy Fleming
1 sibling, 0 replies; 9+ messages in thread
From: Andy Fleming @ 2007-05-21 19:24 UTC (permalink / raw)
To: matvejchikov; +Cc: Jeff Garzik, Randy Dunlap, netdev
On May 11, 2007, at 05:58, Matvejchikov Ilya wrote:
> Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
Acked-by: Andy Fleming <afleming@freescale.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-05-21 19:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-05-10 17:29 [PATCH] phylib: phy_ethtool_{sset,gset} -- check phydev for NULL Matvejchikov Ilya
2007-05-10 17:56 ` Randy Dunlap
2007-05-10 18:19 ` Jeff Garzik
2007-05-11 10:58 ` Matvejchikov Ilya
2007-05-11 13:18 ` Kumar Gala
2007-05-11 13:49 ` Matvejchikov Ilya
2007-05-11 14:44 ` Kumar Gala
2007-05-14 19:32 ` Matvejchikov Ilya
2007-05-21 19:24 ` Andy Fleming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).