Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 21 Nov 2023 10:09:01 +0100
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Christian Marangi <ansuelsmth@gmail.com>
CC:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [net-next PATCH] net: phy: correctly check soft_reset ret ONLY
 if defined for PHY
Message-ID: <ZVxzrWAWO2m4dV7g@lzaremba-mobl.ger.corp.intel.com>
References: <20231120131540.9442-1-ansuelsmth@gmail.com>
 <ZVuUP797C4PLzJsP@lzaremba-mobl.ger.corp.intel.com>
 <655bb796.050a0220.750ad.21a4@mx.google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <655bb796.050a0220.750ad.21a4@mx.google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?siH7dFIYo6sQaIMp60Wxh1zexdigR3wjNhPI+eVoIepCzo+Cz3ED12TuI+3r?=
 =?us-ascii?Q?2ObLcSk6KLc04ZURu4potUg6NTJ23LprR+s34K27bi+4daYhNUNtmdpL1j2l?=
 =?us-ascii?Q?NSI/Jpz8NkecCujCVS2RuW0j/l94xESmz3RDnslrOJv+CpyrfyjeF42rj9od?=
 =?us-ascii?Q?3ONjHpGv83Q8exn0dL80MPi+YyGnR4EQkVfXrSSS9vwLGx10MIW/65rROG+s?=
 =?us-ascii?Q?amp+IXxxFnpKRJUr9rv7hZ4EwI2YpbORrqNkxOE1/EusWaNfvZ42f5vfwrU+?=
 =?us-ascii?Q?NKR7b/qzSeioeJeNcTJUF22oW+rAcUCv4u9BsGoHAb+Qaeg5PGDLuusT7WY5?=
 =?us-ascii?Q?1obtLABZkCsP4KfjkNujhmB2SL45mj7EGAgh+E+JhK7QdcFdxuXNULMy0Y+c?=
 =?us-ascii?Q?45vcLUO8JKZNPFEP94OFNORA7HbR9sCv9xYOHvisC8fj27w8rxn968TliqI2?=
 =?us-ascii?Q?4lwSLzC6CaJMwvFbc74brAlR/ZYVjicMzWd7ue2qcYqF8fU8+npFO6tera8h?=
 =?us-ascii?Q?JHVO7U4V3BqxGvlZ512QT6u7ebCyt63NdF7nEnADXxreBR6r5yKNRYQaWHAR?=
 =?us-ascii?Q?5q3F+F29gXLP/RP0rV+7ZQ6w2+1ugkbBqw75/1Khz5egtptAoKgeSaCcoRE2?=
 =?us-ascii?Q?VmcCxc6JyTHjMFBAHT7RhIHpUyj13i3gtnE5D8Msw5w1LmNHX8YXU2Yuf0VP?=
 =?us-ascii?Q?+cGLMLEQfaiUs1hvmmNTDQTjyH2U+uR1BEyQqluGHXckasWDJ5eJJNwXQG03?=
 =?us-ascii?Q?Xhvbc8HHck8LTFK6lAQ0Q2620R35TansNr/TloOyTzo1MaE7ytyWfqgO/qMw?=
 =?us-ascii?Q?g4t5hb9+warW8H4p4pvF78vD7V0G3gIN86iy2/gCEeUJZmTchFJFVRk8gzbO?=
 =?us-ascii?Q?uYuJEj/+NxSBOhnng+o7a2C49bq0zXQxUb2Y8tzT09fQ+svSFfieuiC9fkNS?=
 =?us-ascii?Q?k7EAurqjImvQSyNOyBrcEfIvPIe26FJIlHhqBoPLbz3jHe0ISfRGkw2igae0?=
 =?us-ascii?Q?lpprNaVeidSUkC11ivbSdIEWlyy19ak3wvOEbk9nRlzKQTt1FT9YTtb72rNT?=
 =?us-ascii?Q?5S8UMGg1foJM7XPI/fgFBhvQpXAJYqm3xf60KEPTPp3LycNwRJKvls6IN9MZ?=
 =?us-ascii?Q?fzy1RzcmzVlbKfINJx+eoCtw5NyTyeD2d3b3DGHjgixXjR2SaczAoVsziOOh?=
 =?us-ascii?Q?L/cTURPn3LSvfhSjF6ih1Q3shXoSlKs+qLNooHHde4Y95S6ktVEUDEklaG8d?=
 =?us-ascii?Q?lx6krkrA0+vM7ELHwyf819qwCNvlWh+/n9r3M5mCMlRXHZRMebi/HJAjH8OL?=
 =?us-ascii?Q?PklyDtyUAnv0BSNsdesF03Xbpn+1Drkgo1Xl93AqRvt49yc5uTMdPPQn4m2r?=
 =?us-ascii?Q?AMi2wrYGZD0oyy4vpzzQijvL/4eyJSh1KVALcscK3+XEzQiA/WFzx1IyCfvv?=
 =?us-ascii?Q?fXXQsV+lr4QOcx1VgHpZJbX483LV5p9hM2I7s8l6I7fsJjVp+fIWYLlop+FO?=
 =?us-ascii?Q?uc13nm3JtIRlaWilUZt3KVZHbjYSDC58ojepMRCi2qvaspoVMSTebjhXxQ17?=
 =?us-ascii?Q?DQrZVW0WPllly7gVBLCMcaiOv3TNapP5Uw7+GLiRYyXgL3uzEM1AyfNn99Rh?=
 =?us-ascii?Q?Vg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 84c74502-0e66-456b-a45f-08dbea7184d9
X-MS-Exchange-CrossTenant-AuthSource: SN7PR11MB7540.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2023 09:09:08.4998
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uh05Acx79BekeI+DIYSXryFKZlZAzGY7rAO9HiHR+u3yoChCRuOAZOTK8TFtKJ+1Z4aQ9WazoHb0BX/J9mbGtiKgdZX02YGdeE7oiu40t3k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6623
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Tue, 21 Nov 2023 01:09:37 -0800 (PST)

On Mon, Nov 20, 2023 at 05:49:28PM +0100, Christian Marangi wrote:
> On Mon, Nov 20, 2023 at 06:15:43PM +0100, Larysa Zaremba wrote:
> > On Mon, Nov 20, 2023 at 02:15:40PM +0100, Christian Marangi wrote:
> > > soft_reset call for phy_init_hw had multiple revision across the years
> > > and the implementation goes back to 2014. Originally was a simple call
> > > to write the generic PHY reset BIT, it was then moved to a dedicated
> > > function. It was then added the option for PHY driver to define their
> > > own special way to reset the PHY. Till this change, checking for ret was
> > > correct as it was always filled by either the generic reset or the
> > > custom implementation. This changed tho with commit 6e2d85ec0559 ("net:
> > > phy: Stop with excessive soft reset"), as the generic reset call to PHY
> > > was dropped but the ret check was never made entirely optional and
> > > dependent whether soft_reset was defined for the PHY driver or not.
> > > 
> > > Luckly nothing was ever added before the soft_reset call so the ret
> > > check (in the case where a PHY didn't had soft_reset defined) although
> > > wrong, never caused problems as ret was init 0 at the start of
> > > phy_init_hw.
> > > 
> > > To prevent any kind of problem and to make the function cleaner and more
> > > robust, correctly move the ret check if the soft_reset section making it
> > > optional and needed only with the function defined.
> > > 
> > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset")
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/net/phy/phy_device.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index 2ce74593d6e4..478126f6b5bc 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev)
> > >  
> > >  	if (phydev->drv->soft_reset) {
> > >  		ret = phydev->drv->soft_reset(phydev);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > >  		/* see comment in genphy_soft_reset for an explanation */
> > > -		if (!ret)
> > > -			phydev->suspended = 0;
> > > +		phydev->suspended = 0;
> > 
> > Are you sure 'suspended' should not be cleared, if soft_reset fails?
> > 
> > >  	}
> 
> Idea is not change current implementation... And before this, suspended
> was cleared only with reset not failing.
> 

Ah, OK. I should not do reviews that late.
If the change is purely cosmetic though, I think Fixes tag does not apply here.

Other than that:

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

But I am also not certain, if maintainers would take such small of a change.

> > >  
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > >  	ret = phy_scan_fixups(phydev);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -- 
> > > 2.40.1
> > > 
> > > 
> 
> -- 
> 	Ansuel
>