Re: [PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

Florian Fainelli
2023-05-17T18:47:37Z

[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]

+tglx, Rafael,

On 5/17/23 05:32, Andrew Lunn wrote:
> On Tue, May 16, 2023 at 03:56:39PM -0700, Florian Fainelli wrote:
>> From: Florian Fainelli <florian.fainelli@broadcom.com>
>>
>> In order to have our interrupt descriptor fully setup, and in particular
>> the action, ensure that we register a full fledged interrupt handler.
>> This is in particular necessary for the kernel to properly manage early
>> wake-up scenarios and arm the wake-up interrupt, otherwise there would
>> be risks of missing the interrupt and leaving it in a state where it
>> would not be handled correctly at all, including for wake-up purposes.
>
> Hi Florian
>
> I've not seen any other driver do this. Maybe that is just my
> ignorance.

As a matter of fact I think this is how most, if not all drivers do it,
they always have an interrupt service routine registered with the
interrupt on which {disable,enable}_irq_wake() is called.

If you remember in my case we do not want to actually service the
interrupt because as soon as we configure the PHY with a wake-up
pattern, the PHY will assert the interrupt line, and if we configure an
unicast/broadcast/multicast pattern we will be interrupted for every
packet received in the network.

If we do not acknowledge the pattern match by doing a clear on read of
the interrupt status register in the PHY, then obviously no new pattern
matched events are generated. The interrupt is level low driven FWIW.

This was the reason why I went with this approach.

>
> Please could you Cc: the interrupt and power management
> Maintainers. This seems like a generic problem, and should have a
> generic solution.

While I was working on this patch set initially, I had missed a call to
irq_set_irq_type(.., IRQ_TYPE_LEVEL_LOW) and the interrupt was left in
its default configuration of being falling edge triggered. The hardware
interrupt generated by the PHY is level low driven. Since we are not
using the interrupt for anything, it did not really matter that the flow
handling would have been incorrect and it worked for the most part.

Except in one particular case which was when I entered an ACPI S5 /
poweroff state, then woke-up the system using the Ethernet PHY, cold
booted the kernel. The GPIO driver would have probed and acknowledged
the interrupt because we want to report any GPIO-based wake-up from ACPI S5:

https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpio/gpio-brcmstb.c#L707

In that cold boot case the PHY driver would probe and acknowledge do the
necessary clear on read and also charge the device for wake-up.

Later any attempt to wake-up from the PHY would not work however. This
came down to the fact that in kernel/irq/pm.c::suspend_device_irq we had
no action associated with the interrupt and therefore we would not be
ensuring that the interrupt was marked as wake-up active within the
interrupt controller provider driver (GPIO).

Maybe there is an opportunity for a patch here to issue a WARN_ON() when
we find an interrupt we call {disable,enable}_irq_wake() against which
does not have an action?

Anyway, I think that the registering a dummy handler is a more correct
way that does not make assumptions about how the interrupt subsystem works.

>
> In the setup which needs this, does the output from the PHY go to a
> PMIC, not a SoC interrupt? And i assume the PMIC is not interrupt
> capable?

The PHY is connected to an always-on GPIO controller which generates an
interrupt output to an out of band interrupt controller that wakes-up
the SoC.
--
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]