* [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts
@ 2017-11-12 21:08 Heiner Kallweit
2017-11-12 21:15 ` [PATCH RfC 2/2] net: phy: core: don't disable device interrupts in phy_change Heiner Kallweit
2017-11-15 22:04 ` [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Florian Fainelli
0 siblings, 2 replies; 6+ messages in thread
From: Heiner Kallweit @ 2017-11-12 21:08 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.
Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.
On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy.c | 26 ++------------------------
include/linux/phy.h | 1 -
2 files changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2b1e67bc1..b3784c9a2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
if (PHY_HALTED == phydev->state)
return IRQ_NONE; /* It can't be ours. */
- disable_irq_nosync(irq);
- atomic_inc(&phydev->irq_disable);
-
phy_change(phydev);
return IRQ_HANDLED;
@@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
*/
int phy_start_interrupts(struct phy_device *phydev)
{
- atomic_set(&phydev->irq_disable, 0);
if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
IRQF_ONESHOT | IRQF_SHARED,
phydev_name(phydev), phydev) < 0) {
@@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
free_irq(phydev->irq, phydev);
- /* If work indeed has been cancelled, disable_irq() will have
- * been left unbalanced from phy_interrupt() and enable_irq()
- * has to be called so that other devices on the line work.
- */
- while (atomic_dec_return(&phydev->irq_disable) >= 0)
- enable_irq(phydev->irq);
-
return err;
}
EXPORT_SYMBOL(phy_stop_interrupts);
@@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
if (phy_interrupt_is_valid(phydev)) {
if (phydev->drv->did_interrupt &&
!phydev->drv->did_interrupt(phydev))
- goto ignore;
+ return;
if (phy_disable_interrupts(phydev))
goto phy_err;
@@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
mutex_unlock(&phydev->lock);
if (phy_interrupt_is_valid(phydev)) {
- atomic_dec(&phydev->irq_disable);
- enable_irq(phydev->irq);
-
/* Reenable interrupts */
if (PHY_HALTED != phydev->state &&
phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
- goto irq_enable_err;
+ goto phy_err;
}
/* reschedule state queue work to run as soon as possible */
phy_trigger_machine(phydev, true);
return;
-ignore:
- atomic_dec(&phydev->irq_disable);
- enable_irq(phydev->irq);
- return;
-
-irq_enable_err:
- disable_irq(phydev->irq);
- atomic_inc(&phydev->irq_disable);
phy_err:
phy_error(phydev);
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb..8a87e441f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -468,7 +468,6 @@ struct phy_device {
/* Interrupt and Polling infrastructure */
struct work_struct phy_queue;
struct delayed_work state_queue;
- atomic_t irq_disable;
struct mutex lock;
--
2.15.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH RfC 2/2] net: phy: core: don't disable device interrupts in phy_change
2017-11-12 21:08 [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Heiner Kallweit
@ 2017-11-12 21:15 ` Heiner Kallweit
2017-11-15 22:04 ` [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Florian Fainelli
1 sibling, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2017-11-12 21:15 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
If state is not PHY_HALTED I see no need to temporarily disable
interrupts on the device. As long as the current interrupt isn't acked
on the device no new interrupt can happen anyway.
In addition remove a unneeded enabling of interrupts in the state
machine when handling state PHY_CHANGELINK.
Tested on a Odroid-C2 with RTL8211F phy in interrupt mode.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b3784c9a2..4a11de8cb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -727,8 +727,9 @@ void phy_change(struct phy_device *phydev)
!phydev->drv->did_interrupt(phydev))
return;
- if (phy_disable_interrupts(phydev))
- goto phy_err;
+ if (phydev->state == PHY_HALTED)
+ if (phy_disable_interrupts(phydev))
+ goto phy_err;
}
mutex_lock(&phydev->lock);
@@ -736,15 +737,11 @@ void phy_change(struct phy_device *phydev)
phydev->state = PHY_CHANGELINK;
mutex_unlock(&phydev->lock);
- if (phy_interrupt_is_valid(phydev)) {
- /* Reenable interrupts */
- if (PHY_HALTED != phydev->state &&
- phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
- goto phy_err;
- }
-
/* reschedule state queue work to run as soon as possible */
phy_trigger_machine(phydev, true);
+
+ if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
+ goto phy_err;
return;
phy_err:
@@ -984,10 +981,6 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
phy_link_down(phydev, true);
}
-
- if (phy_interrupt_is_valid(phydev))
- err = phy_config_interrupt(phydev,
- PHY_INTERRUPT_ENABLED);
break;
case PHY_HALTED:
if (phydev->link) {
--
2.15.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts
2017-11-12 21:08 [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Heiner Kallweit
2017-11-12 21:15 ` [PATCH RfC 2/2] net: phy: core: don't disable device interrupts in phy_change Heiner Kallweit
@ 2017-11-15 22:04 ` Florian Fainelli
2017-11-15 22:19 ` Heiner Kallweit
1 sibling, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-11-15 22:04 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev, Ard Biesheuvel
On 11/12/2017 01:08 PM, Heiner Kallweit wrote:
> After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
> sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
> some simplification" all relevant code pieces run in process context
> anyway and I don't think we need the disabling of interrupts any longer.
>
> Interestingly enough, latter commit already removed the comment
> explaining why interrupts need to be temporarily disabled.
>
> On my system phy interrupt mode works fine with this patch.
> However I may miss something, especially in the context of shared phy
> interrupts, therefore I'd appreciate if more people could test this.
I am not currently in a position to test this, but this looks very
similar, if not identical to what Ard submitted a few days earlier:
https://patchwork.kernel.org/patch/10048901/
Since net-next is closed at the moment, that should allow us to give
this some good amount of testing.
Thanks
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy.c | 26 ++------------------------
> include/linux/phy.h | 1 -
> 2 files changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 2b1e67bc1..b3784c9a2 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> if (PHY_HALTED == phydev->state)
> return IRQ_NONE; /* It can't be ours. */
>
> - disable_irq_nosync(irq);
> - atomic_inc(&phydev->irq_disable);
> -
> phy_change(phydev);
>
> return IRQ_HANDLED;
> @@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
> */
> int phy_start_interrupts(struct phy_device *phydev)
> {
> - atomic_set(&phydev->irq_disable, 0);
> if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
> IRQF_ONESHOT | IRQF_SHARED,
> phydev_name(phydev), phydev) < 0) {
> @@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
>
> free_irq(phydev->irq, phydev);
>
> - /* If work indeed has been cancelled, disable_irq() will have
> - * been left unbalanced from phy_interrupt() and enable_irq()
> - * has to be called so that other devices on the line work.
> - */
> - while (atomic_dec_return(&phydev->irq_disable) >= 0)
> - enable_irq(phydev->irq);
> -
> return err;
> }
> EXPORT_SYMBOL(phy_stop_interrupts);
> @@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
> if (phy_interrupt_is_valid(phydev)) {
> if (phydev->drv->did_interrupt &&
> !phydev->drv->did_interrupt(phydev))
> - goto ignore;
> + return;
>
> if (phy_disable_interrupts(phydev))
> goto phy_err;
> @@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
> mutex_unlock(&phydev->lock);
>
> if (phy_interrupt_is_valid(phydev)) {
> - atomic_dec(&phydev->irq_disable);
> - enable_irq(phydev->irq);
> -
> /* Reenable interrupts */
> if (PHY_HALTED != phydev->state &&
> phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
> - goto irq_enable_err;
> + goto phy_err;
> }
>
> /* reschedule state queue work to run as soon as possible */
> phy_trigger_machine(phydev, true);
> return;
>
> -ignore:
> - atomic_dec(&phydev->irq_disable);
> - enable_irq(phydev->irq);
> - return;
> -
> -irq_enable_err:
> - disable_irq(phydev->irq);
> - atomic_inc(&phydev->irq_disable);
> phy_err:
> phy_error(phydev);
> }
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index dc82a07cb..8a87e441f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -468,7 +468,6 @@ struct phy_device {
> /* Interrupt and Polling infrastructure */
> struct work_struct phy_queue;
> struct delayed_work state_queue;
> - atomic_t irq_disable;
>
> struct mutex lock;
>
>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts
2017-11-15 22:04 ` [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Florian Fainelli
@ 2017-11-15 22:19 ` Heiner Kallweit
2017-11-16 9:51 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2017-11-15 22:19 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev, Ard Biesheuvel
Am 15.11.2017 um 23:04 schrieb Florian Fainelli:
> On 11/12/2017 01:08 PM, Heiner Kallweit wrote:
>> After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
>> sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
>> some simplification" all relevant code pieces run in process context
>> anyway and I don't think we need the disabling of interrupts any longer.
>>
>> Interestingly enough, latter commit already removed the comment
>> explaining why interrupts need to be temporarily disabled.
>>
>> On my system phy interrupt mode works fine with this patch.
>> However I may miss something, especially in the context of shared phy
>> interrupts, therefore I'd appreciate if more people could test this.
>
> I am not currently in a position to test this, but this looks very
> similar, if not identical to what Ard submitted a few days earlier:
>
Thanks for the hint. Indeed it's exactly the same patch, so the one
sent by me can be disregarded.
> https://patchwork.kernel.org/patch/10048901/
>
> Since net-next is closed at the moment, that should allow us to give
> this some good amount of testing.
>
> Thanks
>
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/phy.c | 26 ++------------------------
>> include/linux/phy.h | 1 -
>> 2 files changed, 2 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 2b1e67bc1..b3784c9a2 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>> if (PHY_HALTED == phydev->state)
>> return IRQ_NONE; /* It can't be ours. */
>>
>> - disable_irq_nosync(irq);
>> - atomic_inc(&phydev->irq_disable);
>> -
>> phy_change(phydev);
>>
>> return IRQ_HANDLED;
>> @@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
>> */
>> int phy_start_interrupts(struct phy_device *phydev)
>> {
>> - atomic_set(&phydev->irq_disable, 0);
>> if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
>> IRQF_ONESHOT | IRQF_SHARED,
>> phydev_name(phydev), phydev) < 0) {
>> @@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
>>
>> free_irq(phydev->irq, phydev);
>>
>> - /* If work indeed has been cancelled, disable_irq() will have
>> - * been left unbalanced from phy_interrupt() and enable_irq()
>> - * has to be called so that other devices on the line work.
>> - */
>> - while (atomic_dec_return(&phydev->irq_disable) >= 0)
>> - enable_irq(phydev->irq);
>> -
>> return err;
>> }
>> EXPORT_SYMBOL(phy_stop_interrupts);
>> @@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
>> if (phy_interrupt_is_valid(phydev)) {
>> if (phydev->drv->did_interrupt &&
>> !phydev->drv->did_interrupt(phydev))
>> - goto ignore;
>> + return;
>>
>> if (phy_disable_interrupts(phydev))
>> goto phy_err;
>> @@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
>> mutex_unlock(&phydev->lock);
>>
>> if (phy_interrupt_is_valid(phydev)) {
>> - atomic_dec(&phydev->irq_disable);
>> - enable_irq(phydev->irq);
>> -
>> /* Reenable interrupts */
>> if (PHY_HALTED != phydev->state &&
>> phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
>> - goto irq_enable_err;
>> + goto phy_err;
>> }
>>
>> /* reschedule state queue work to run as soon as possible */
>> phy_trigger_machine(phydev, true);
>> return;
>>
>> -ignore:
>> - atomic_dec(&phydev->irq_disable);
>> - enable_irq(phydev->irq);
>> - return;
>> -
>> -irq_enable_err:
>> - disable_irq(phydev->irq);
>> - atomic_inc(&phydev->irq_disable);
>> phy_err:
>> phy_error(phydev);
>> }
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index dc82a07cb..8a87e441f 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -468,7 +468,6 @@ struct phy_device {
>> /* Interrupt and Polling infrastructure */
>> struct work_struct phy_queue;
>> struct delayed_work state_queue;
>> - atomic_t irq_disable;
>>
>> struct mutex lock;
>>
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts
2017-11-15 22:19 ` Heiner Kallweit
@ 2017-11-16 9:51 ` Ard Biesheuvel
2017-11-29 20:50 ` Heiner Kallweit
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-11-16 9:51 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, Andrew Lunn, David Miller,
<netdev@vger.kernel.org>
On 15 November 2017 at 22:19, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 15.11.2017 um 23:04 schrieb Florian Fainelli:
>> On 11/12/2017 01:08 PM, Heiner Kallweit wrote:
>>> After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
>>> sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
>>> some simplification" all relevant code pieces run in process context
>>> anyway and I don't think we need the disabling of interrupts any longer.
>>>
>>> Interestingly enough, latter commit already removed the comment
>>> explaining why interrupts need to be temporarily disabled.
>>>
>>> On my system phy interrupt mode works fine with this patch.
>>> However I may miss something, especially in the context of shared phy
>>> interrupts, therefore I'd appreciate if more people could test this.
>>
>> I am not currently in a position to test this, but this looks very
>> similar, if not identical to what Ard submitted a few days earlier:
>>
>
> Thanks for the hint. Indeed it's exactly the same patch, so the one
> sent by me can be disregarded.
>
Well, it does appear your patch is more complete. Another difference
is that I actually need this change to fix an issue with a
hierarchical irqchip stacked on top of the GIC.
>> https://patchwork.kernel.org/patch/10048901/
>>
>> Since net-next is closed at the moment, that should allow us to give
>> this some good amount of testing.
>>
>> Thanks
>>
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
For the record
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Dear maintainers,
Please take whichever of these patches looks more correct to you.
Thanks,
Ard.
>>> ---
>>> drivers/net/phy/phy.c | 26 ++------------------------
>>> include/linux/phy.h | 1 -
>>> 2 files changed, 2 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index 2b1e67bc1..b3784c9a2 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>>> if (PHY_HALTED == phydev->state)
>>> return IRQ_NONE; /* It can't be ours. */
>>>
>>> - disable_irq_nosync(irq);
>>> - atomic_inc(&phydev->irq_disable);
>>> -
>>> phy_change(phydev);
>>>
>>> return IRQ_HANDLED;
>>> @@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
>>> */
>>> int phy_start_interrupts(struct phy_device *phydev)
>>> {
>>> - atomic_set(&phydev->irq_disable, 0);
>>> if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
>>> IRQF_ONESHOT | IRQF_SHARED,
>>> phydev_name(phydev), phydev) < 0) {
>>> @@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
>>>
>>> free_irq(phydev->irq, phydev);
>>>
>>> - /* If work indeed has been cancelled, disable_irq() will have
>>> - * been left unbalanced from phy_interrupt() and enable_irq()
>>> - * has to be called so that other devices on the line work.
>>> - */
>>> - while (atomic_dec_return(&phydev->irq_disable) >= 0)
>>> - enable_irq(phydev->irq);
>>> -
>>> return err;
>>> }
>>> EXPORT_SYMBOL(phy_stop_interrupts);
>>> @@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
>>> if (phy_interrupt_is_valid(phydev)) {
>>> if (phydev->drv->did_interrupt &&
>>> !phydev->drv->did_interrupt(phydev))
>>> - goto ignore;
>>> + return;
>>>
>>> if (phy_disable_interrupts(phydev))
>>> goto phy_err;
>>> @@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
>>> mutex_unlock(&phydev->lock);
>>>
>>> if (phy_interrupt_is_valid(phydev)) {
>>> - atomic_dec(&phydev->irq_disable);
>>> - enable_irq(phydev->irq);
>>> -
>>> /* Reenable interrupts */
>>> if (PHY_HALTED != phydev->state &&
>>> phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
>>> - goto irq_enable_err;
>>> + goto phy_err;
>>> }
>>>
>>> /* reschedule state queue work to run as soon as possible */
>>> phy_trigger_machine(phydev, true);
>>> return;
>>>
>>> -ignore:
>>> - atomic_dec(&phydev->irq_disable);
>>> - enable_irq(phydev->irq);
>>> - return;
>>> -
>>> -irq_enable_err:
>>> - disable_irq(phydev->irq);
>>> - atomic_inc(&phydev->irq_disable);
>>> phy_err:
>>> phy_error(phydev);
>>> }
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index dc82a07cb..8a87e441f 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -468,7 +468,6 @@ struct phy_device {
>>> /* Interrupt and Polling infrastructure */
>>> struct work_struct phy_queue;
>>> struct delayed_work state_queue;
>>> - atomic_t irq_disable;
>>>
>>> struct mutex lock;
>>>
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts
2017-11-16 9:51 ` Ard Biesheuvel
@ 2017-11-29 20:50 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2017-11-29 20:50 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller
Cc: Ard Biesheuvel, <netdev@vger.kernel.org>
Am 16.11.2017 um 10:51 schrieb Ard Biesheuvel:
> On 15 November 2017 at 22:19, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Am 15.11.2017 um 23:04 schrieb Florian Fainelli:
>>> On 11/12/2017 01:08 PM, Heiner Kallweit wrote:
>>>> After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
>>>> sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
>>>> some simplification" all relevant code pieces run in process context
>>>> anyway and I don't think we need the disabling of interrupts any longer.
>>>>
>>>> Interestingly enough, latter commit already removed the comment
>>>> explaining why interrupts need to be temporarily disabled.
>>>>
>>>> On my system phy interrupt mode works fine with this patch.
>>>> However I may miss something, especially in the context of shared phy
>>>> interrupts, therefore I'd appreciate if more people could test this.
>>>
>>> I am not currently in a position to test this, but this looks very
>>> similar, if not identical to what Ard submitted a few days earlier:
>>>
>>
>> Thanks for the hint. Indeed it's exactly the same patch, so the one
>> sent by me can be disregarded.
>>
>
> Well, it does appear your patch is more complete. Another difference
> is that I actually need this change to fix an issue with a
> hierarchical irqchip stacked on top of the GIC.
>
>
>>> https://patchwork.kernel.org/patch/10048901/
>>>
>>> Since net-next is closed at the moment, that should allow us to give
>>> this some good amount of testing.
>>>
>>> Thanks
>>>
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>
> For the record
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Dear maintainers,
>
> Please take whichever of these patches looks more correct to you.
>
> Thanks,
> Ard.
>
These two patches have status RFC in patchwork. Based on Ard's review
and comment, any action to be taken from his or my side?
Rgds, Heiner
>>>> ---
>>>> drivers/net/phy/phy.c | 26 ++------------------------
>>>> include/linux/phy.h | 1 -
>>>> 2 files changed, 2 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>> index 2b1e67bc1..b3784c9a2 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>>>> if (PHY_HALTED == phydev->state)
>>>> return IRQ_NONE; /* It can't be ours. */
>>>>
>>>> - disable_irq_nosync(irq);
>>>> - atomic_inc(&phydev->irq_disable);
>>>> -
>>>> phy_change(phydev);
>>>>
>>>> return IRQ_HANDLED;
>>>> @@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
>>>> */
>>>> int phy_start_interrupts(struct phy_device *phydev)
>>>> {
>>>> - atomic_set(&phydev->irq_disable, 0);
>>>> if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
>>>> IRQF_ONESHOT | IRQF_SHARED,
>>>> phydev_name(phydev), phydev) < 0) {
>>>> @@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
>>>>
>>>> free_irq(phydev->irq, phydev);
>>>>
>>>> - /* If work indeed has been cancelled, disable_irq() will have
>>>> - * been left unbalanced from phy_interrupt() and enable_irq()
>>>> - * has to be called so that other devices on the line work.
>>>> - */
>>>> - while (atomic_dec_return(&phydev->irq_disable) >= 0)
>>>> - enable_irq(phydev->irq);
>>>> -
>>>> return err;
>>>> }
>>>> EXPORT_SYMBOL(phy_stop_interrupts);
>>>> @@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
>>>> if (phy_interrupt_is_valid(phydev)) {
>>>> if (phydev->drv->did_interrupt &&
>>>> !phydev->drv->did_interrupt(phydev))
>>>> - goto ignore;
>>>> + return;
>>>>
>>>> if (phy_disable_interrupts(phydev))
>>>> goto phy_err;
>>>> @@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
>>>> mutex_unlock(&phydev->lock);
>>>>
>>>> if (phy_interrupt_is_valid(phydev)) {
>>>> - atomic_dec(&phydev->irq_disable);
>>>> - enable_irq(phydev->irq);
>>>> -
>>>> /* Reenable interrupts */
>>>> if (PHY_HALTED != phydev->state &&
>>>> phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
>>>> - goto irq_enable_err;
>>>> + goto phy_err;
>>>> }
>>>>
>>>> /* reschedule state queue work to run as soon as possible */
>>>> phy_trigger_machine(phydev, true);
>>>> return;
>>>>
>>>> -ignore:
>>>> - atomic_dec(&phydev->irq_disable);
>>>> - enable_irq(phydev->irq);
>>>> - return;
>>>> -
>>>> -irq_enable_err:
>>>> - disable_irq(phydev->irq);
>>>> - atomic_inc(&phydev->irq_disable);
>>>> phy_err:
>>>> phy_error(phydev);
>>>> }
>>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>>> index dc82a07cb..8a87e441f 100644
>>>> --- a/include/linux/phy.h
>>>> +++ b/include/linux/phy.h
>>>> @@ -468,7 +468,6 @@ struct phy_device {
>>>> /* Interrupt and Polling infrastructure */
>>>> struct work_struct phy_queue;
>>>> struct delayed_work state_queue;
>>>> - atomic_t irq_disable;
>>>>
>>>> struct mutex lock;
>>>>
>>>>
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-29 20:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-12 21:08 [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Heiner Kallweit
2017-11-12 21:15 ` [PATCH RfC 2/2] net: phy: core: don't disable device interrupts in phy_change Heiner Kallweit
2017-11-15 22:04 ` [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Florian Fainelli
2017-11-15 22:19 ` Heiner Kallweit
2017-11-16 9:51 ` Ard Biesheuvel
2017-11-29 20:50 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox