Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: David Miller Date: Fri, 03 Apr 2015 15:04:10 -0400 (EDT) > Ok I'll apply this mvneta patch to net-next then, thanks. Nevermind I just saw in my patchwork queue that there is a v3 of this change set that adds some adjustments to the fixed PHY interfaces. I think I'll apply that series instead of this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: Florian Fainelli
Date: Thu, 02 Apr 2015 18:03:53 -0700
> On 02/04/15 17:51, David Miller wrote:
>> From: Stas Sergeev
>> Date: Tue, 31 Mar 2015 16:24:59 +0300
>>
>>> @@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
>>>
>>> static void mvneta_mdio_remove(struct mvneta_port *pp)
>>> {
>>> + fixed_phy_set_link_update(pp->phy_dev, NULL);
>>
>> I do not see any other driver doing this on shutdown.
>> Please show me why it is necessary.
>
> The primary reason is that if you do not do that, past the point where
> you call phy_disconnect(), we stop the PHY state machine, detach from
> the net_device, such that it won't invoke the adjust_link callback
> anymore. The fixed PHY driver, though will still keep calling the
> fixed_link_update callback asking the driver whether the link parameters
> need to be updated, and that will just cause a NULL pointer de-reference
> phydev->attached_dev, since we are now in detached state.
>
> I guess another way to fix that is to look for the PHY state in
> fixed_mdio_read() and do nothing if it is PHY_HALTED.
Ok I'll apply this mvneta patch to net-next then, thanks.
>> And if it is, all other drivers registering a fixed phy link update
>> function need to be adjusted to do the same thing.
>
> I think the bcmgenet driver is now doing this as a result of Petri's
> latest changes, and I meant to comment on that before the patch got in.
> drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke
> phy_disconnect() nor can be rmmod'd, so a lesser issue.
I just seems insane to me that phy_disconnect() doesn't stop the
callbacks from running.
Fixed PHY seems to me to suffer from a lack of proper integration
into the PHY layer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
03.04.2015 03:51, David Miller пишет:
> From: Stas Sergeev
> Date: Tue, 31 Mar 2015 16:24:59 +0300
>
>> @@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
>>
>> static void mvneta_mdio_remove(struct mvneta_port *pp)
>> {
>> +fixed_phy_set_link_update(pp->phy_dev, NULL);
>
> I do not see any other driver doing this on shutdown.
> Please show me why it is necessary.
Hello David, sorry for this being discussed in a different thread,
so here you have a few pointers to why is this needed:
https://lkml.org/lkml/2015/3/30/367
> And if it is, all other drivers registering a fixed phy link update
> function need to be adjusted to do the same thing.
Or, for example, get rid of the callback and add an API:
http://www.spinics.net/lists/netdev/msg323517.html
Then you get the patch that avoids all this call-back singing and dancing:
http://www.spinics.net/lists/netdev/msg323518.html
In any case, I posted 3 different implementations and hope
people will choose the best one (any choice is fine with me).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
03.04.2015 03:51, David Miller пишет:
From: Stas Sergeev s...@list.ru
Date: Tue, 31 Mar 2015 16:24:59 +0300
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
static void mvneta_mdio_remove(struct mvneta_port *pp)
{
+fixed_phy_set_link_update(pp-phy_dev, NULL);
I do not see any other driver doing this on shutdown.
Please show me why it is necessary.
Hello David, sorry for this being discussed in a different thread,
so here you have a few pointers to why is this needed:
https://lkml.org/lkml/2015/3/30/367
And if it is, all other drivers registering a fixed phy link update
function need to be adjusted to do the same thing.
Or, for example, get rid of the callback and add an API:
http://www.spinics.net/lists/netdev/msg323517.html
Then you get the patch that avoids all this call-back singing and dancing:
http://www.spinics.net/lists/netdev/msg323518.html
In any case, I posted 3 different implementations and hope
people will choose the best one (any choice is fine with me).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: Florian Fainelli f.faine...@gmail.com
Date: Thu, 02 Apr 2015 18:03:53 -0700
On 02/04/15 17:51, David Miller wrote:
From: Stas Sergeev s...@list.ru
Date: Tue, 31 Mar 2015 16:24:59 +0300
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
static void mvneta_mdio_remove(struct mvneta_port *pp)
{
+ fixed_phy_set_link_update(pp-phy_dev, NULL);
I do not see any other driver doing this on shutdown.
Please show me why it is necessary.
The primary reason is that if you do not do that, past the point where
you call phy_disconnect(), we stop the PHY state machine, detach from
the net_device, such that it won't invoke the adjust_link callback
anymore. The fixed PHY driver, though will still keep calling the
fixed_link_update callback asking the driver whether the link parameters
need to be updated, and that will just cause a NULL pointer de-reference
phydev-attached_dev, since we are now in detached state.
I guess another way to fix that is to look for the PHY state in
fixed_mdio_read() and do nothing if it is PHY_HALTED.
Ok I'll apply this mvneta patch to net-next then, thanks.
And if it is, all other drivers registering a fixed phy link update
function need to be adjusted to do the same thing.
I think the bcmgenet driver is now doing this as a result of Petri's
latest changes, and I meant to comment on that before the patch got in.
drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke
phy_disconnect() nor can be rmmod'd, so a lesser issue.
I just seems insane to me that phy_disconnect() doesn't stop the
callbacks from running.
Fixed PHY seems to me to suffer from a lack of proper integration
into the PHY layer.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: David Miller da...@davemloft.net Date: Fri, 03 Apr 2015 15:04:10 -0400 (EDT) Ok I'll apply this mvneta patch to net-next then, thanks. Nevermind I just saw in my patchwork queue that there is a v3 of this change set that adds some adjustments to the fixed PHY interfaces. I think I'll apply that series instead of this patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
On 02/04/15 17:51, David Miller wrote:
> From: Stas Sergeev
> Date: Tue, 31 Mar 2015 16:24:59 +0300
>
>> @@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
>>
>> static void mvneta_mdio_remove(struct mvneta_port *pp)
>> {
>> +fixed_phy_set_link_update(pp->phy_dev, NULL);
>
> I do not see any other driver doing this on shutdown.
> Please show me why it is necessary.
The primary reason is that if you do not do that, past the point where
you call phy_disconnect(), we stop the PHY state machine, detach from
the net_device, such that it won't invoke the adjust_link callback
anymore. The fixed PHY driver, though will still keep calling the
fixed_link_update callback asking the driver whether the link parameters
need to be updated, and that will just cause a NULL pointer de-reference
phydev->attached_dev, since we are now in detached state.
I guess another way to fix that is to look for the PHY state in
fixed_mdio_read() and do nothing if it is PHY_HALTED.
>
> And if it is, all other drivers registering a fixed phy link update
> function need to be adjusted to do the same thing.
>
I think the bcmgenet driver is now doing this as a result of Petri's
latest changes, and I meant to comment on that before the patch got in.
drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke
phy_disconnect() nor can be rmmod'd, so a lesser issue.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: Stas Sergeev
Date: Tue, 31 Mar 2015 16:24:59 +0300
> @@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
>
> static void mvneta_mdio_remove(struct mvneta_port *pp)
> {
> + fixed_phy_set_link_update(pp->phy_dev, NULL);
I do not see any other driver doing this on shutdown.
Please show me why it is necessary.
And if it is, all other drivers registering a fixed phy link update
function need to be adjusted to do the same thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
From: Stas Sergeev s...@list.ru
Date: Tue, 31 Mar 2015 16:24:59 +0300
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
static void mvneta_mdio_remove(struct mvneta_port *pp)
{
+ fixed_phy_set_link_update(pp-phy_dev, NULL);
I do not see any other driver doing this on shutdown.
Please show me why it is necessary.
And if it is, all other drivers registering a fixed phy link update
function need to be adjusted to do the same thing.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mvneta: implement SGMII-based in-band link state signaling
On 02/04/15 17:51, David Miller wrote:
From: Stas Sergeev s...@list.ru
Date: Tue, 31 Mar 2015 16:24:59 +0300
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
static void mvneta_mdio_remove(struct mvneta_port *pp)
{
+fixed_phy_set_link_update(pp-phy_dev, NULL);
I do not see any other driver doing this on shutdown.
Please show me why it is necessary.
The primary reason is that if you do not do that, past the point where
you call phy_disconnect(), we stop the PHY state machine, detach from
the net_device, such that it won't invoke the adjust_link callback
anymore. The fixed PHY driver, though will still keep calling the
fixed_link_update callback asking the driver whether the link parameters
need to be updated, and that will just cause a NULL pointer de-reference
phydev-attached_dev, since we are now in detached state.
I guess another way to fix that is to look for the PHY state in
fixed_mdio_read() and do nothing if it is PHY_HALTED.
And if it is, all other drivers registering a fixed phy link update
function need to be adjusted to do the same thing.
I think the bcmgenet driver is now doing this as a result of Petri's
latest changes, and I meant to comment on that before the patch got in.
drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke
phy_disconnect() nor can be rmmod'd, so a lesser issue.
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
