diff mbox series

[v4,1/5] net: macb: fix wakeup test in runtime suspend/resume routines

Message ID dc30ff1d17cb5a75ddd10966eab001f67ac744ef.1588763703.git.nicolas.ferre@microchip.com (mailing list archive)
State New, archived
Headers show
Series net: macb: Wake-on-Lan magic packet fixes and GEM handling | expand

Commit Message

Nicolas Ferre May 6, 2020, 11:37 a.m. UTC 
From: Nicolas Ferre <nicolas.ferre@microchip.com>

Use the proper struct device pointer to check if the wakeup flag
and wakeup source are positioned.
Use the one passed by function call which is equivalent to
&bp->dev->dev.parent.

It's preventing the trigger of a spurious interrupt in case the
Wake-on-Lan feature is used.

Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Harini Katakam <harini.katakam@xilinx.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski May 6, 2020, 8:18 p.m. UTC | #1 
On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Use the proper struct device pointer to check if the wakeup flag
> and wakeup source are positioned.
> Use the one passed by function call which is equivalent to
> &bp->dev->dev.parent.
> 
> It's preventing the trigger of a spurious interrupt in case the
> Wake-on-Lan feature is used.
> 
> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")

        Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
        Has these problem(s):
                - Target SHA1 does not exist

> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Harini Katakam <harini.katakam@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 36290a8e2a84..d11fae37d46b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev)
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
>  
> -	if (!(device_may_wakeup(&bp->dev->dev))) {
> +	if (!(device_may_wakeup(dev))) {
>  		clk_disable_unprepare(bp->tx_clk);
>  		clk_disable_unprepare(bp->hclk);
>  		clk_disable_unprepare(bp->pclk);
> @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev)
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
>  
> -	if (!(device_may_wakeup(&bp->dev->dev))) {
> +	if (!(device_may_wakeup(dev))) {
>  		clk_prepare_enable(bp->pclk);
>  		clk_prepare_enable(bp->hclk);
>  		clk_prepare_enable(bp->tx_clk);
Nicolas Ferre May 7, 2020, 10:03 a.m. UTC | #2 
On 06/05/2020 at 22:18, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote:
>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Use the proper struct device pointer to check if the wakeup flag
>> and wakeup source are positioned.
>> Use the one passed by function call which is equivalent to
>> &bp->dev->dev.parent.
>>
>> It's preventing the trigger of a spurious interrupt in case the
>> Wake-on-Lan feature is used.
>>
>> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
> 
>          Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
>          Has these problem(s):
>                  - Target SHA1 does not exist

Indeed, it's:
Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support")

David: do I have to respin or you can modify it?

Thanks. Regards,
   Nicolas
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Cc: Harini Katakam <harini.katakam@xilinx.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 36290a8e2a84..d11fae37d46b 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev)
>>        struct net_device *netdev = dev_get_drvdata(dev);
>>        struct macb *bp = netdev_priv(netdev);
>>
>> -     if (!(device_may_wakeup(&bp->dev->dev))) {
>> +     if (!(device_may_wakeup(dev))) {
>>                clk_disable_unprepare(bp->tx_clk);
>>                clk_disable_unprepare(bp->hclk);
>>                clk_disable_unprepare(bp->pclk);
>> @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev)
>>        struct net_device *netdev = dev_get_drvdata(dev);
>>        struct macb *bp = netdev_priv(netdev);
>>
>> -     if (!(device_may_wakeup(&bp->dev->dev))) {
>> +     if (!(device_may_wakeup(dev))) {
>>                clk_prepare_enable(bp->pclk);
>>                clk_prepare_enable(bp->hclk);
>>                clk_prepare_enable(bp->tx_clk);
>
Nicolas Ferre May 25, 2020, 8:18 a.m. UTC | #3 
On 07/05/2020 at 12:03, Nicolas Ferre wrote:
> On 06/05/2020 at 22:18, Jakub Kicinski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote:
>>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>>
>>> Use the proper struct device pointer to check if the wakeup flag
>>> and wakeup source are positioned.
>>> Use the one passed by function call which is equivalent to
>>> &bp->dev->dev.parent.
>>>
>>> It's preventing the trigger of a spurious interrupt in case the
>>> Wake-on-Lan feature is used.
>>>
>>> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
>>
>>           Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
>>           Has these problem(s):
>>                   - Target SHA1 does not exist
> 
> Indeed, it's:
> Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support")
> 
> David: do I have to respin or you can modify it?

David, all, I'm about to resend this series (alternative to "ping"), 
however:

1/ Now that it's late in the cycle, I'd like that you tell me if I 
rebase on net-next because it isn't not sensible to queue such (non 
urgeent) changes at rc7

2/ I didn't get answers from Russell and can't tell if there's a better 
way of handling underlying phylink error of phylink_ethtool_set_wol() in 
patch 3/5

Best regards,
   Nicolas

>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> Cc: Harini Katakam <harini.katakam@xilinx.com>
>>> ---
>>>    drivers/net/ethernet/cadence/macb_main.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index 36290a8e2a84..d11fae37d46b 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev)
>>>         struct net_device *netdev = dev_get_drvdata(dev);
>>>         struct macb *bp = netdev_priv(netdev);
>>>
>>> -     if (!(device_may_wakeup(&bp->dev->dev))) {
>>> +     if (!(device_may_wakeup(dev))) {
>>>                 clk_disable_unprepare(bp->tx_clk);
>>>                 clk_disable_unprepare(bp->hclk);
>>>                 clk_disable_unprepare(bp->pclk);
>>> @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev)
>>>         struct net_device *netdev = dev_get_drvdata(dev);
>>>         struct macb *bp = netdev_priv(netdev);
>>>
>>> -     if (!(device_may_wakeup(&bp->dev->dev))) {
>>> +     if (!(device_may_wakeup(dev))) {
>>>                 clk_prepare_enable(bp->pclk);
>>>                 clk_prepare_enable(bp->hclk);
>>>                 clk_prepare_enable(bp->tx_clk);
>>
> 
>
Russell King (Oracle) May 25, 2020, 8:47 a.m. UTC | #4 
On Mon, May 25, 2020 at 10:18:16AM +0200, Nicolas Ferre wrote:
> On 07/05/2020 at 12:03, Nicolas Ferre wrote:
> > On 06/05/2020 at 22:18, Jakub Kicinski wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote:
> > > > From: Nicolas Ferre <nicolas.ferre@microchip.com>
> > > > 
> > > > Use the proper struct device pointer to check if the wakeup flag
> > > > and wakeup source are positioned.
> > > > Use the one passed by function call which is equivalent to
> > > > &bp->dev->dev.parent.
> > > > 
> > > > It's preventing the trigger of a spurious interrupt in case the
> > > > Wake-on-Lan feature is used.
> > > > 
> > > > Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
> > > 
> > >           Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
> > >           Has these problem(s):
> > >                   - Target SHA1 does not exist
> > 
> > Indeed, it's:
> > Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support")
> > 
> > David: do I have to respin or you can modify it?
> 
> David, all, I'm about to resend this series (alternative to "ping"),
> however:
> 
> 1/ Now that it's late in the cycle, I'd like that you tell me if I rebase on
> net-next because it isn't not sensible to queue such (non urgeent) changes
> at rc7
> 
> 2/ I didn't get answers from Russell and can't tell if there's a better way
> of handling underlying phylink error of phylink_ethtool_set_wol() in patch
> 3/5

I think you could have answered your own questions there, but I seemed
easier to send an email.  I've just read the code, typed out an
appropriate description of the code's behaviour, and then derived the
answer to your questions without anything else.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 36290a8e2a84..d11fae37d46b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4616,7 +4616,7 @@  static int __maybe_unused macb_runtime_suspend(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 
-	if (!(device_may_wakeup(&bp->dev->dev))) {
+	if (!(device_may_wakeup(dev))) {
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
@@ -4632,7 +4632,7 @@  static int __maybe_unused macb_runtime_resume(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 
-	if (!(device_may_wakeup(&bp->dev->dev))) {
+	if (!(device_may_wakeup(dev))) {
 		clk_prepare_enable(bp->pclk);
 		clk_prepare_enable(bp->hclk);
 		clk_prepare_enable(bp->tx_clk);