linux-kernel - Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup

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: <a01fa1eb-be77-4da1-bfa5-3e3c20bc08a7@arinc9.com>
Date: Sun, 17 Dec 2023 15:42:34 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Daniel Golle <daniel@...rotopia.org>,
 Landen Chao <Landen.Chao@...iatek.com>, DENG Qingfang <dqfext@...il.com>,
 Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
 Florian Fainelli <f.fainelli@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Bartel Eerdekens <bartel.eerdekens@...stell8.be>, mithat.guner@...ont.com,
 erkin.bozoglu@...ont.com
Subject: Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set
 priv->p5_interface on mt7530_setup_port5()

On 7.12.2023 21:18, Vladimir Oltean wrote:
> On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
>> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
>> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
>> setting priv->p5_interface would prevent mt7530_setup_port5() from running
>> more than once.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 069b3dfca6fa..fc87ec817672 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>>   	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>>   		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>>   
>> -	priv->p5_interface = interface;
>> -
>>   unlock_exit:
>>   	mutex_unlock(&priv->reg_mutex);
>>   }
>> -- 
>> 2.40.1
>>
> 
> Purely as a matter of theory, mt753x_phylink_mac_config() itself could
> be called more than once, at any time there is a phylink_major_config() -
> first during initial one, then upon any change of the phy_interface_t.
> With the port being fixed and internal, maybe that is unlikely.
> 
> Destroying and re-creating the phylink instance could also make the
> driver see more than 1 mt753x_phylink_mac_config() call. During periods
> of -EPROBE_DEFER, maybe this could even happen.
> 
> I think what we need to see is a description of what the priv->p5_interface
> test is really protecting against, because it certainly is uncommon in other
> drivers to have this much logic to avoid mt753x_mac_config() being
> called twice.
> 
> If there's nothing wrong with calling it twice and it's just an optimization,
> I think that's enough of a justification for removing that extra protection.
> At the same time, the optimization (and simplification) that we should
> pursue is that we should remove the overlap between phylink and the
> initial port setup made by the driver.

priv->p5_interface and priv->p6_interface were actually intended to apply
only for MT7531. It prevents the CPU ports to be configured again. CPU
ports are unnecessarily configured before phylink. I intend to address that
with the patch below. I'll submit it after the current patches here go
through. There's a lot to clean up in this driver.

https://github.com/arinc9/linux/commit/80efcb1870530ef5526d7feda625374b8f308369

If you can recall, this is where me and Russell mostly left off on my 30
patch series. I was supposed to run some debug info prints to make sure
that the MT7531 CPU ports are still configured as they were with
priv->info->cpu_port_config().

https://lore.kernel.org/netdev/ZHy2jQLesdYFMQtO@shell.armlinux.org.uk/

For this patch, I can change the patch log to state that priv->p5_interface
and priv->p6_interface are intended for use on the MT7531 switch.

Arınç