netdev - Re: [RFC PATCH 1/2] net: phy: let the driver register its own IRQ handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <18f531a691d0c4905552794bbb1be1e5@walle.cc>
Date:   Wed, 26 Feb 2020 12:12:15 +0100
From:   Michael Walle <michael@...le.cc>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [RFC PATCH 1/2] net: phy: let the driver register its own IRQ
 handler

Am 2020-02-26 08:27, schrieb Heiner Kallweit:
> On 26.02.2020 00:08, Michael Walle wrote:
>> There are more and more PHY drivers which has more than just the PHY
>> link change interrupts. For example, temperature thresholds or PTP
>> interrupts.
>> 
>> At the moment it is not possible to correctly handle interrupts for 
>> PHYs
>> which has a clear-on-read interrupt status register. It is also likely
>> that the current approach of the phylib isn't working for all PHYs out
>> there.
>> 
>> Therefore, this patch let the PHY driver register its own interrupt
>> handler. To notify the phylib about a link change, the interrupt 
>> handler
>> has to call the new function phy_drv_interrupt().
>> 
> 
> We have phy_driver callback handle_interrupt for custom interrupt
> handlers. Any specific reason why you can't use it for your purposes?

Yes, as mentioned above this wont work for PHYs which has a 
clear-on-read
status register, because you may loose interrupts between 
handle_interrupt()
and phy_clear_interrupt().

See also
  
https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/

And esp. Russell reply. I tried using handle_interrupt() but it won't 
work
unless you make the ack_interrupt a NOOP. but even then it won't work 
because
the ack_interrupt is also used during setup etc.

>> Signed-off-by: Michael Walle <michael@...le.cc>
>> ---
>>  drivers/net/phy/phy.c        | 15 +++++++++++++++
>>  drivers/net/phy/phy_device.c |  6 ++++--
>>  include/linux/phy.h          |  2 ++
>>  3 files changed, 21 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d76e038cf2cb..f25aacbcf1d9 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -942,6 +942,21 @@ void phy_mac_interrupt(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(phy_mac_interrupt);
>> 
>> +/**
>> + * phy_drv_interrupt - PHY drivers says the link has changed
>> + * @phydev: phy_device struct with changed link
>> + *
>> + * The PHY driver may implement his own interrupt handler. It will 
>> call this
>> + * function to notify us about a link change. Trigger the state 
>> machine and
>> + * work a work queue.
>> + */
> 
> This function would duplicate phy_mac_interrupt().

Yes I wasn't sure, if I should just call that function of if it might
change in the future. Also the description doesn't fit. Renaming the 
function
sounded also bad, because its an exported symbol.

I'm open to suggestions ;)

> 
>> +void phy_drv_interrupt(struct phy_device *phydev)
>> +{
>> +	/* Trigger a state machine change */
>> +	phy_trigger_machine(phydev);
>> +}
>> +EXPORT_SYMBOL(phy_drv_interrupt);
>> +
>>  static void mmd_eee_adv_to_linkmode(unsigned long *advertising, u16 
>> eee_adv)
>>  {
>>  	linkmode_zero(advertising);
>> diff --git a/drivers/net/phy/phy_device.c 
>> b/drivers/net/phy/phy_device.c
>> index 6a5056e0ae77..6d8c94e61251 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -965,7 +965,8 @@ int phy_connect_direct(struct net_device *dev, 
>> struct phy_device *phydev,
>>  		return rc;
>> 
>>  	phy_prepare_link(phydev, handler);
>> -	if (phy_interrupt_is_valid(phydev))
>> +	if (phy_interrupt_is_valid(phydev) &&
>> +	    phydev->drv->flags & PHY_HAS_OWN_IRQ_HANDLER)
>>  		phy_request_interrupt(phydev);
> 
> Here most likely a ! is missing. because as-is you would break
> current phylib interrupt mode.

whoops you're right. nice catch.

> Where in the PHY driver (in which
> callback) do you want to register your own interrupt handler?

In the _probe() see patch 2/2.

-michael

>> 
>>  	return 0;
>> @@ -2411,7 +2412,8 @@ EXPORT_SYMBOL(phy_validate_pause);
>> 
>>  static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>>  {
>> -	return phydrv->config_intr && phydrv->ack_interrupt;
>> +	return ((phydrv->config_intr && phydrv->ack_interrupt) ||
>> +		phydrv->flags & PHY_HAS_OWN_IRQ_HANDLER);
>>  }
>> 
>>  /**
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index c570e162e05e..46f73b94fd60 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -75,6 +75,7 @@ extern const int phy_10gbit_features_array[1];
>> 
>>  #define PHY_IS_INTERNAL		0x00000001
>>  #define PHY_RST_AFTER_CLK_EN	0x00000002
>> +#define PHY_HAS_OWN_IRQ_HANDLER	0x00000004
>>  #define MDIO_DEVICE_IS_PHY	0x80000000
>> 
>>  /* Interface Mode definitions */
>> @@ -1235,6 +1236,7 @@ int phy_drivers_register(struct phy_driver 
>> *new_driver, int n,
>>  void phy_state_machine(struct work_struct *work);
>>  void phy_queue_state_machine(struct phy_device *phydev, unsigned long 
>> jiffies);
>>  void phy_mac_interrupt(struct phy_device *phydev);
>> +void phy_drv_interrupt(struct phy_device *phydev);
>>  void phy_start_machine(struct phy_device *phydev);
>>  void phy_stop_machine(struct phy_device *phydev);
>>  void phy_ethtool_ksettings_get(struct phy_device *phydev,
>>