Am 16.11.2017 um 10:51 schrieb Ard Biesheuvel:
> On 15 November 2017 at 22:19, Heiner Kallweit <hkallwe...@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 <hkallwe...@gmail.com>
>
> For the record
>
> Acked-by: Ard Biesheuvel <ard.biesheu...@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;
>>>>
>>>>
>>>
>>>
>>
>
