netdev - Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

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

Date:   Fri, 16 Dec 2022 10:46:08 +0100
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Xu Liang <lxu@...linear.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on
 GPY215 by default

Am 2022-12-03 21:36, schrieb Andrew Lunn:
>> > > @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev)
>> > >  	phydev->priv = priv;
>> > >  	mutex_init(&priv->mbox_lock);
>> > >
>> > > +	if (gpy_has_broken_mdint(phydev) &&
>> > > +	    !device_property_present(dev,
>> > > "maxlinear,use-broken-interrupts"))
>> > > +		phydev->irq = PHY_POLL;
>> > > +
>> >
>> > I'm not sure of ordering here. It could be phydev->irq is set after
>> > probe. The IRQ is requested as part of phy_connect_direct(), which is
>> > much later.
>> 
>> I've did it that way, because phy_probe() also sets phydev->irq = 
>> PHY_POLL
>> in some cases and the phy driver .probe() is called right after it.
> 
> Yes, it is a valid point to do this check, but on its own i don't
> think it is sufficient.

Care to elaborate a bit? E.g. what is the difference to the case
the phy would have an interrupt described but no .config_intr()
op.

>> > I think a better place for this test is in gpy_config_intr(), return
>> > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
>> > phy_request_interrupt() to use polling.
>> 
>> Which will then print a warning, which might be misleading.
>> Or we disable the warning if -EOPNOTSUPP is returned?
> 
> Disabling the warning is the right thing to do.

There is more to this. .config_intr() is also used in
phy_init_hw() and phy_drv_supports_irq(). The latter would
still return true in our case. I'm not sure that is correct.

After trying your suggestion, I'm still in favor of somehow
tell the phy core to force polling mode during probe() of the
driver. The same way it's done if there is no .config_intr().

It's not like we'd need change the mode after probe during
runtime. Also with your proposed changed the attachment print
is wrong/misleading as it still prints the original irq instead
of PHY_POLL.

-michael