Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   <Divya.Koppera@microchip.com>
To:     <andrew@lunn.ch>
CC:     <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <richardcochran@gmail.com>,
        <UNGLinuxDriver@microchip.com>
Subject: RE: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero
 to PTR_ERR
Thread-Topic: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero
 to PTR_ERR
Thread-Index: AQHZCUVX+xhZw8rF5EesOiBOrBuDqa5g1PgAgAAJjpCAAAeBgIAI7uug
Date:   Mon, 12 Dec 2022 08:34:44 +0000
Message-ID: <CO1PR11MB4771F8AA1CCAD01EAE797E59E2E29@CO1PR11MB4771.namprd11.prod.outlook.com>
References: <20221206073511.4772-1-Divya.Koppera@microchip.com>
 <20221206073511.4772-3-Divya.Koppera@microchip.com>
 <Y48+rLpF7Gre/s1P@lunn.ch>
 <CO1PR11MB47713C125F3D0E08B7A6A132E21B9@CO1PR11MB4771.namprd11.prod.outlook.com>
 <Y49M++waEHLm0hEA@lunn.ch>
In-Reply-To: <Y49M++waEHLm0hEA@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?aX5ho2XZGOEQ8xWONwOLwi4FlfZWAdNKzThyXrYWOotZLgkWAY9xaAWIYhLE?=
 =?us-ascii?Q?APVxrbn3/w1VR5qb+cOdGnFFI19Uh2vz3fbga9TjuuDAva4CVZHiEme3Kzdh?=
 =?us-ascii?Q?K00/IRAU8x5xQYE57iHFqsFw7n7zUyJyh78QFm0lAGkAYHMzC/Q8X6iU/Sh7?=
 =?us-ascii?Q?Y+GdVaVZNdOlYfzHEJk/1lO1oyvVEYP+b+0Jlbwmv7YNz4+aBU86D4WLFpdP?=
 =?us-ascii?Q?i/2BPZ9wrIOqIDJxDB/q3BRU06fthpGl6OYGQBTYOrHvq4ylaAgf1mMoVgDD?=
 =?us-ascii?Q?jYCl25MTKDHdbmzevlh5meqwLKYNu2tyHAEUY5wx+ck3Qey0NsOnHW6mjNNg?=
 =?us-ascii?Q?U/4hkU/0HRkiCcIebB8YN5MMl3gWLBoMm21aZepM9BySAO311SWhsJdmuhjr?=
 =?us-ascii?Q?vysRvHc+zz1uPQGm25PFKlF9XyShp1zZ7TH/uo1rpPtoZmWhgzxcLi0N+2M6?=
 =?us-ascii?Q?+LYSpoTzFPQ8uBYBUFm8RsPIgkGhdsO1qfCfHLE9Pt/0g6GDAsMRKyyCc7mb?=
 =?us-ascii?Q?tZdSYdPV3frMOu41CprcpWVoHA2e3GRlp1lY9todajCVTTOuZCCLPoxI0K1+?=
 =?us-ascii?Q?k38TyfAH5c5qBgmMxJycPRWzEfanjmckMzAdS0IlSSzMlvG6ix2xmQL5RHYq?=
 =?us-ascii?Q?YsrTUKJGN/7P92Ce223CgaaJJqPA7zm5uFkWuRKZbyGzzk8CTVDncWAUpM20?=
 =?us-ascii?Q?7fui9BzDcyRuLG4mRJxepCKjCPWJ7UpknhXH5rfg00xkvSKEcQjVbzzKBmTb?=
 =?us-ascii?Q?nqwp6ypH5UCJB5f892NOev+984mjCswweDTlkxoO3Bl4lf28Z5F61yPi91ad?=
 =?us-ascii?Q?EleUvD9/xx/XTH4O3A6V/u7AJYW+ETTKaqVKpsn9z5uqHrDGF+PcdEzdVKAv?=
 =?us-ascii?Q?VLMj7RUFhLDwO/suj1NO1ntnYlsV60izYQU2yCXwq7S7JBL2/J9tR5sYldoI?=
 =?us-ascii?Q?9GGQlTmMx9xWgqmKtSeVmHxUdeivLCO7r2fH4GGHU1HPgvL+ylrvKUGaCjAz?=
 =?us-ascii?Q?kD9PYGntwLQDpO8+fWgv6lKSBwbQw54WWQZfN+m/RPYTopuq/ObuHqOn5kci?=
 =?us-ascii?Q?mt2I+FFEmav2Aki+5Udi63hsvpj797le2vbczZ6+kohQdqxJTPgPFpynf0yF?=
 =?us-ascii?Q?vV+wD+/15dyY5Nfox09v/0n9qyEXCAZEyws1Q1NKH3ErsIZQ4GuxGwDozwVC?=
 =?us-ascii?Q?o6uCUCw1EPbRavYKKiPG56/63vbnw8Xukuk7unubHiuc0WIRpiq/+djzNvn9?=
 =?us-ascii?Q?htcwPH+geBKXRodFc2hQcuzcY0TLasQ3jyKOcGnnBCNT40jpHn1lMAETVHHQ?=
 =?us-ascii?Q?grabrxDNl8ocsIGLTJss70BWmJP3okelyxPgCH8hqhSNa28KzR0QvRG6Bvs9?=
 =?us-ascii?Q?mK19iunxpvxeEFCAjBNBuUASqHnHiURXq/JKrq0mhDtsISE8aDMek1GIGEEK?=
 =?us-ascii?Q?iDD77xqHVo4SGsGf6w/7UIC8I4nnA1YJd+3+dszhogCmAg1P7Kxr3fzGZVn3?=
 =?us-ascii?Q?zEngjQrL5VjOG7xyUjddbU0unN5Vcfh43f0YgLO/jhb/G8LhiKDvboFxALv7?=
 =?us-ascii?Q?wUp1ha8HEHLVNHalVKWrcczbK2NLgW/UVDkF4easJgpVxQjN2kZ205lbqmOs?=
 =?us-ascii?Q?ag=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4771.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e95b1087-7887-41f2-8806-08dadc1bb86c
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Dec 2022 08:34:44.2069
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: aRhhMCx1xmFOlbcLf1fvywtByO7JtYSmELiI6oeRq1ktSu+wlaB251XQ+rLXu76ucs9Phd86y/LTzHSL+9oYYcCXN3Uidp4l/3hDEotX0KA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6771
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, December 6, 2022 7:39 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> richardcochran@gmail.com; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing =
zero
> to PTR_ERR
>=20
> EXTERNAL EMAIL: Do not click links or open attachments unless you know th=
e
> content is safe
>=20
> > > > -     if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > > > -         !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > > > -             return 0;
> > > > -
> > >
> > > Why are you removing this ?
> > >
> >
> > I got review comment from Richard in v2 as below, making it as consiste=
nt
> by checking ptp_clock. So removed it in next revision.
> >
> > " > static int lan8814_ptp_probe_once(struct phy_device *phydev)
> > > {
> > >         struct lan8814_shared_priv *shared =3D phydev->shared->priv;
> > >
> > >         if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > >             !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > >                 return 0;
> >
> > It is weird to use macros here, but not before calling ptp_clock_regist=
er.
> > Make it consistent by checking shared->ptp_clock instead.
> > That is also better form."
>=20
> O.K. If Richard said this fine.
>=20
> Just out of interest, could you disassemble lan8814_ptp_probe_once() when
> CONFIG_PTP_1588_CLOCK is disabled, with and without this check?
>=20

If I understand correctly,

With (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) check, initialization of mutex and=
 members of shared->ptp_clock_info need to be done in first function.
Without above check ptp_clock_register should be done in second function. C=
orrect me if I'm wrong.

In this case, if first function is bypassed because of clock disable config=
, no need to go to second function. Could you please check below solution, =
if that works fine?

> My guess is, when PTP is disabled, the mutex still gets initialised, all =
the
> member of shared->ptp_clock_info are set. The optimise cannot remove it.
> With the macro check, the function is empty. So you end up with a slightl=
y
> bigger text size.
>=20

If that is the case, I'll keep the CONFIG_PTP_1588_CLOCK disabled check bef=
ore calling lan8814_ptp_probe_once.

Next thing is if CONFIG_PTP_1588_CLOCK disabled check pass then ptp_clock_r=
egister will never return null because we are bypassing hardware clock chec=
k before calling function itself.
So, I can remove ptp_clock null check too. Let me know if this works, I'll =
do changes in next revision.

>        Andrew