[PATCH net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling

[PATCH net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling

[Radu Pirea (OSS)]

.config_intr() handles only the link event interrupt and should also
disable/enable the PTP interrupt.

Even if the PTP irqs bit is toggled unconditionally, it is safe. This
interrupt acts as a global switch for all PTP irqs. By default, this bit
is set.

Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support")
CC: stable@vger.kernel.org # 5.15+
Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/phy/nxp-c45-tja11xx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 5813b07242ce..4d7f4cb05f89 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -63,6 +63,9 @@
 #define VEND1_PORT_ABILITIES		0x8046
 #define PTP_ABILITY			BIT(3)
 
+#define VEND1_PORT_FUNC_IRQ_EN		0x807A
+#define PTP_IRQS			BIT(3)
+
 #define VEND1_PORT_INFRA_CONTROL	0xAC00
 #define PORT_INFRA_CONTROL_EN		BIT(14)
 
@@ -890,12 +893,15 @@ static int nxp_c45_start_op(struct phy_device *phydev)
 
 static int nxp_c45_config_intr(struct phy_device *phydev)
 {
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS);
 		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
 					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
-	else
+	} else {
+		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS);
 		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
 					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+	}
 }
 
 static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
-- 
2.34.1

Re: [PATCH net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling

[Jakub Kicinski]

On Mon, 10 Apr 2023 15:48:56 +0300 Radu Pirea (OSS) wrote:
> -	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS);

Isn't the third argument supposed to be the address?
Am I missing something or this patch was no tested properly?

Also why ignore the return value?

>  		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
>  					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> -	else
> +	} else {
> +		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS);
>  		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
>  					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> +	}

Re: [PATCH net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling

[Radu Pirea (OSS)]

On 13.04.2023 06:44, Jakub Kicinski wrote:
> On Mon, 10 Apr 2023 15:48:56 +0300 Radu Pirea (OSS) wrote:
>> -	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
>> +		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS);
> 
> Isn't the third argument supposed to be the address?
> Am I missing something or this patch was no tested properly?
Yes, you are right. Thank you for this catch. I discovered this fix 
based on a driver code review and it did not trigger any issues. I just 
wanted to be sure if the PTP irqs are left in an inconsistent state, 
they are disabled from the kill switch.

I will send a v2.

> 
> Also why ignore the return value?
This register might not be present on every PHY, that's why the return 
value is ignored.

Radu P.

[...]

Re: [PATCH net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling

[Andrew Lunn]

> > Also why ignore the return value?
> This register might not be present on every PHY, that's why the return value
> is ignored.

Please document that, otherwise you might see people add code to check
the return value. Or better, still, differentiate between the
different versions, and only touch it when it does exist.

	  Andtrew