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

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- 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 --]