Re: [PATCH 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Neil Armstrong]

> > void phy_disconnect(struct phy_device *phydev)
> > {
> > if (phydev->irq > 0)
> > phy_stop_interrupts(phydev);
> >
> > phy_stop_machine(phydev);
> >
> > phydev->adjust_link = NULL;
> >
> > phy_detach(phydev);
> > }
> 
> And this does not yet get called. It probably needs to be in
> dsa_switch_destroy() just before unregister_netdev() of the slave
> devices.
> 
> However, the ordering in dsa_switch_destroy() looks wrong. The fixed
> phys are destroyed before the slave devices. They should probably be
> destroyed after the slave devices, or at least after the
> phy_disconnect() is called.
> 
>  Andrew
> 

Andrew, Florian,

Thanks for the review, a call to phy_disconnect was missing in 
dsa_switch_destroy.

I will post a new patchset with the correct fix, a switch to delayed_work and
a separate dsa_slave_destroy function for sake of maintenance ease.

Neil

--
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Neil Armstrong]

> > void phy_disconnect(struct phy_device *phydev)
> > {
> > if (phydev->irq > 0)
> > phy_stop_interrupts(phydev);
> >
> > phy_stop_machine(phydev);
> >
> > phydev->adjust_link = NULL;
> >
> > phy_detach(phydev);
> > }
> 
> And this does not yet get called. It probably needs to be in
> dsa_switch_destroy() just before unregister_netdev() of the slave
> devices.
> 
> However, the ordering in dsa_switch_destroy() looks wrong. The fixed
> phys are destroyed before the slave devices. They should probably be
> destroyed after the slave devices, or at least after the
> phy_disconnect() is called.
> 
>  Andrew
> 

Andrew, Florian,

Thanks for the review, a call to phy_disconnect was missing in 
dsa_switch_destroy.

I will post a new patchset with the correct fix, a switch to delayed_work and
a separate dsa_slave_destroy function for sake of maintenance ease.

Neil

--
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Andrew Lunn]

On Tue, Oct 27, 2015 at 08:57:58AM -0700, Florian Fainelli wrote:
> (don't top post please)
> 
> On 27/10/15 08:53, Frode Isaksen wrote:
> > What will you need in the oops ? I presume you don' want everything or ?
> > 
> > The PHY state machine is not stopped with a PHY disconnect.
> 
> It is stopped with a phy_disconnect():
> 
> /**
>  * phy_disconnect - disable interrupts, stop state machine, and detach a PHY
>  *  device
>  * @phydev: target phy_device struct
>  */
> void phy_disconnect(struct phy_device *phydev)
> {
> if (phydev->irq > 0)
> phy_stop_interrupts(phydev);
> 
> phy_stop_machine(phydev);
> 
> phydev->adjust_link = NULL;
> 
> phy_detach(phydev);
> }

And this does not yet get called. It probably needs to be in
dsa_switch_destroy() just before unregister_netdev() of the slave
devices.

However, the ordering in dsa_switch_destroy() looks wrong. The fixed
phys are destroyed before the slave devices. They should probably be
destroyed after the slave devices, or at least after the
phy_disconnect() is called.

 Andrew
--
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Florian Fainelli]

(don't top post please)

On 27/10/15 08:53, Frode Isaksen wrote:
> What will you need in the oops ? I presume you don' want everything or ?
> 
> The PHY state machine is not stopped with a PHY disconnect.

It is stopped with a phy_disconnect():

/**
 * phy_disconnect - disable interrupts, stop state machine, and detach a PHY
 *  device
 * @phydev: target phy_device struct
 */
void phy_disconnect(struct phy_device *phydev)
{
if (phydev->irq > 0)
phy_stop_interrupts(phydev);

phy_stop_machine(phydev);

phydev->adjust_link = NULL;

phy_detach(phydev);
}


> However, the
> 'phy-change' work is cancelled, so cancelling this work in the remove
> function maybe not needed. I will verify ASAP.
> 
> Frode
> 
> 2015-10-27 16:40 GMT+01:00 Florian Fainelli  >:
> 
> On 27/10/15 07:49, Neil Armstrong wrote:
> > Avoids:
> >  Unable to handle kernel NULL pointer dereference at virtual address 
> 0064
> >  Workqueue: events_power_efficient phy_state_machine
> >  PC is at phy_state_machine+0x28/0x480
> 
> Stripped down oops can sometimes be missing critical pieces of
> information to help debug the problem, is there a reason why this is
> being obfuscated?
> 
> You are supposed to stop the PHY state machine by calling
> phy_disconnect() is it possible that this is missing?
> 
> >
> > Signed-off-by: Frode Isaksen  >
> > Signed-off-by: Neil Armstrong  >
> > ---
> >  drivers/net/phy/phy_device.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> b/drivers/net/phy/phy_device.c
> > index 3833891..b5b6c1b 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
> >   phydev->state = PHY_DOWN;
> >   mutex_unlock(>lock);
> >
> > + cancel_delayed_work_sync(>state_queue);
> > + flush_delayed_work(>state_queue);
> > +
> > + cancel_work_sync(>phy_queue);
> > + flush_work(>phy_queue);
> > +
> >   if (phydev->drv->remove)
> >   phydev->drv->remove(phydev);
> >   phydev->drv = NULL;
> >
> 
> 
> --
> Florian
> 
> 


-- 
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Florian Fainelli]

On 27/10/15 07:49, Neil Armstrong wrote:
> Avoids:
>  Unable to handle kernel NULL pointer dereference at virtual address 0064
>  Workqueue: events_power_efficient phy_state_machine
>  PC is at phy_state_machine+0x28/0x480

Stripped down oops can sometimes be missing critical pieces of
information to help debug the problem, is there a reason why this is
being obfuscated?

You are supposed to stop the PHY state machine by calling
phy_disconnect() is it possible that this is missing?

> 
> Signed-off-by: Frode Isaksen 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/net/phy/phy_device.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3833891..b5b6c1b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
>   phydev->state = PHY_DOWN;
>   mutex_unlock(>lock);
> 
> + cancel_delayed_work_sync(>state_queue);
> + flush_delayed_work(>state_queue);
> +
> + cancel_work_sync(>phy_queue);
> + flush_work(>phy_queue);
> +
>   if (phydev->drv->remove)
>   phydev->drv->remove(phydev);
>   phydev->drv = NULL;
> 


-- 
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/

[PATCH 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Neil Armstrong]

Avoids:
 Unable to handle kernel NULL pointer dereference at virtual address 0064
 Workqueue: events_power_efficient phy_state_machine
 PC is at phy_state_machine+0x28/0x480

Signed-off-by: Frode Isaksen 
Signed-off-by: Neil Armstrong 
---
 drivers/net/phy/phy_device.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3833891..b5b6c1b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
phydev->state = PHY_DOWN;
mutex_unlock(>lock);

+   cancel_delayed_work_sync(>state_queue);
+   flush_delayed_work(>state_queue);
+
+   cancel_work_sync(>phy_queue);
+   flush_work(>phy_queue);
+
if (phydev->drv->remove)
phydev->drv->remove(phydev);
phydev->drv = NULL;
-- 
1.9.1
--
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/

[PATCH 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Neil Armstrong]

Avoids:
 Unable to handle kernel NULL pointer dereference at virtual address 0064
 Workqueue: events_power_efficient phy_state_machine
 PC is at phy_state_machine+0x28/0x480

Signed-off-by: Frode Isaksen 
Signed-off-by: Neil Armstrong 
---
 drivers/net/phy/phy_device.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3833891..b5b6c1b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
phydev->state = PHY_DOWN;
mutex_unlock(>lock);

+   cancel_delayed_work_sync(>state_queue);
+   flush_delayed_work(>state_queue);
+
+   cancel_work_sync(>phy_queue);
+   flush_work(>phy_queue);
+
if (phydev->drv->remove)
phydev->drv->remove(phydev);
phydev->drv = NULL;
-- 
1.9.1
--
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Florian Fainelli]

(don't top post please)

On 27/10/15 08:53, Frode Isaksen wrote:
> What will you need in the oops ? I presume you don' want everything or ?
> 
> The PHY state machine is not stopped with a PHY disconnect.

It is stopped with a phy_disconnect():

/**
 * phy_disconnect - disable interrupts, stop state machine, and detach a PHY
 *  device
 * @phydev: target phy_device struct
 */
void phy_disconnect(struct phy_device *phydev)
{
if (phydev->irq > 0)
phy_stop_interrupts(phydev);

phy_stop_machine(phydev);

phydev->adjust_link = NULL;

phy_detach(phydev);
}


> However, the
> 'phy-change' work is cancelled, so cancelling this work in the remove
> function maybe not needed. I will verify ASAP.
> 
> Frode
> 
> 2015-10-27 16:40 GMT+01:00 Florian Fainelli  >:
> 
> On 27/10/15 07:49, Neil Armstrong wrote:
> > Avoids:
> >  Unable to handle kernel NULL pointer dereference at virtual address 
> 0064
> >  Workqueue: events_power_efficient phy_state_machine
> >  PC is at phy_state_machine+0x28/0x480
> 
> Stripped down oops can sometimes be missing critical pieces of
> information to help debug the problem, is there a reason why this is
> being obfuscated?
> 
> You are supposed to stop the PHY state machine by calling
> phy_disconnect() is it possible that this is missing?
> 
> >
> > Signed-off-by: Frode Isaksen  >
> > Signed-off-by: Neil Armstrong  >
> > ---
> >  drivers/net/phy/phy_device.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> b/drivers/net/phy/phy_device.c
> > index 3833891..b5b6c1b 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
> >   phydev->state = PHY_DOWN;
> >   mutex_unlock(>lock);
> >
> > + cancel_delayed_work_sync(>state_queue);
> > + flush_delayed_work(>state_queue);
> > +
> > + cancel_work_sync(>phy_queue);
> > + flush_work(>phy_queue);
> > +
> >   if (phydev->drv->remove)
> >   phydev->drv->remove(phydev);
> >   phydev->drv = NULL;
> >
> 
> 
> --
> Florian
> 
> 


-- 
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Florian Fainelli]

On 27/10/15 07:49, Neil Armstrong wrote:
> Avoids:
>  Unable to handle kernel NULL pointer dereference at virtual address 0064
>  Workqueue: events_power_efficient phy_state_machine
>  PC is at phy_state_machine+0x28/0x480

Stripped down oops can sometimes be missing critical pieces of
information to help debug the problem, is there a reason why this is
being obfuscated?

You are supposed to stop the PHY state machine by calling
phy_disconnect() is it possible that this is missing?

> 
> Signed-off-by: Frode Isaksen 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/net/phy/phy_device.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3833891..b5b6c1b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1348,6 +1348,12 @@ static int phy_remove(struct device *dev)
>   phydev->state = PHY_DOWN;
>   mutex_unlock(>lock);
> 
> + cancel_delayed_work_sync(>state_queue);
> + flush_delayed_work(>state_queue);
> +
> + cancel_work_sync(>phy_queue);
> + flush_work(>phy_queue);
> +
>   if (phydev->drv->remove)
>   phydev->drv->remove(phydev);
>   phydev->drv = NULL;
> 


-- 
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 6/6] net: phy: Stop 'phy-state-machine' and 'phy_change' work on remove

[Andrew Lunn]

On Tue, Oct 27, 2015 at 08:57:58AM -0700, Florian Fainelli wrote:
> (don't top post please)
> 
> On 27/10/15 08:53, Frode Isaksen wrote:
> > What will you need in the oops ? I presume you don' want everything or ?
> > 
> > The PHY state machine is not stopped with a PHY disconnect.
> 
> It is stopped with a phy_disconnect():
> 
> /**
>  * phy_disconnect - disable interrupts, stop state machine, and detach a PHY
>  *  device
>  * @phydev: target phy_device struct
>  */
> void phy_disconnect(struct phy_device *phydev)
> {
> if (phydev->irq > 0)
> phy_stop_interrupts(phydev);
> 
> phy_stop_machine(phydev);
> 
> phydev->adjust_link = NULL;
> 
> phy_detach(phydev);
> }

And this does not yet get called. It probably needs to be in
dsa_switch_destroy() just before unregister_netdev() of the slave
devices.

However, the ordering in dsa_switch_destroy() looks wrong. The fixed
phys are destroyed before the slave devices. They should probably be
destroyed after the slave devices, or at least after the
phy_disconnect() is called.

 Andrew
--
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/