Received-SPF: pass (google.com: domain of linux-kernel+bounces-178507-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Hariprasad Kelam <hkelam@marvell.com>
To: Wei Fang <wei.fang@nxp.com>, "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org"
	<kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        Shenwei Wang
	<shenwei.wang@nxp.com>,
        Clark Wang <xiaoning.wang@nxp.com>,
        "richardcochran@gmail.com" <richardcochran@gmail.com>,
        "andrew@lunn.ch"
	<andrew@lunn.ch>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "imx@lists.linux.dev" <imx@lists.linux.dev>
Subject: RE: [PATCH net] net: fec: avoid lock evasion when reading pps_enable
Thread-Topic: [PATCH net] net: fec: avoid lock evasion when reading pps_enable
Thread-Index: AQHapRfDaiJFn1FWf0uT/Cuv3lOav7GVFuiAgAFb01A=
Date: Tue, 14 May 2024 09:26:07 +0000
Message-ID: 
 <PH0PR18MB44748039C05AE91AAD2DB514DEE32@PH0PR18MB4474.namprd18.prod.outlook.com>
References: <20240513015127.961360-1-wei.fang@nxp.com>
 <PH0PR18MB4474D5050F6CBA0B2D4A6041DEE22@PH0PR18MB4474.namprd18.prod.outlook.com>
 <PAXPR04MB851010A6DBA52A04DA104E1C88E22@PAXPR04MB8510.eurprd04.prod.outlook.com>
In-Reply-To: 
 <PAXPR04MB851010A6DBA52A04DA104E1C88E22@PAXPR04MB8510.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?iso-2022-jp?B?aFZWVk45Y1NiMVpKT3ZJN3VpTk82WU9YbjVpNUgyZ1NoTEwwN1FDNVRI?=
 =?iso-2022-jp?B?MTJ2MkNmVk9vWE1VNmhaMC9nSUhWMXNOaWVVSXFvcHAwbzRzTVNzL3Bk?=
 =?iso-2022-jp?B?ZmZheVdHdlNLK1NneGtZTmt1MUsrWGFnZmJRNFEzZkxjbXFWSTZES3hy?=
 =?iso-2022-jp?B?R0RMcXBqWUxyOXBrdWxlWFN0UVlPM1hZN0hZWlpmNkw0VHNaSE5oYVdL?=
 =?iso-2022-jp?B?L3F6Tk1wQnhRUVVDL2pUbjZpRVQyTC9IWUNQR2pnaCtzeStxcXBFajhE?=
 =?iso-2022-jp?B?QXU3b0xwdEl4NHlsR08vRjlHSWh5Z3R3VkpBQ3FObk9LdUNGcitCNUhI?=
 =?iso-2022-jp?B?UGdWTDMwWC9XQ1Zmc1JWZmtVMEoxTVp3clM2VzgwR2liMmJXeEcvWFdx?=
 =?iso-2022-jp?B?bktCVnM4Qm5NK2p3VDBVRG55ZHJSQ1BXUUdDa1hQZU1CT0lsakh4WEZU?=
 =?iso-2022-jp?B?TG11azlJTW1rZGpxWlFQUitnUkNRdVFNemJEWFVwa1F5dUt0RnVmS0Z5?=
 =?iso-2022-jp?B?a3MvMlJrODhqVDVNRDhSREV2Z1JuVEM5YUVuVWxIaUN2SHkxczV2dDhS?=
 =?iso-2022-jp?B?UlpseUh6VmcyRDdCTlZyOWFjUzBsV0pXdjRnOHBGVGRHaFJuYVlNUFVC?=
 =?iso-2022-jp?B?b2VHR0tUZ0dZUEpWcVdabE1aUXdQSllWakVuamt1NDhjT2c3Y1BDdGMx?=
 =?iso-2022-jp?B?azJobG9PbVpVcVZtV0xMem55L1h6cXpocmQvNFdZcHJJQmFqSkxuT28y?=
 =?iso-2022-jp?B?MnhKeW1CWG0rc25KUmRIVGhFVUZBTUE3ZGZQUTZmQTVLNkJtWnJGTGVa?=
 =?iso-2022-jp?B?QzFZQnE3TVRDenNKYnhHbjBOZDY2c2podzlSdysxUEppajMyU01EV3pM?=
 =?iso-2022-jp?B?U0E2ZnlEaVYvdnY1ZWdSTmVSdks5OThROExKaTJ0a2xDNCtwNDhEOTlU?=
 =?iso-2022-jp?B?VHlOZVhaQWJPcXlOYW1JNmxYbEsrb2cwTHc2R2NHdEMyaVFwSDdkNUZz?=
 =?iso-2022-jp?B?OHdTcm5DNVVVUzhGV2gxalBnd2xEa0I2YjRIMTc5RnArVTlHc3BlT3FW?=
 =?iso-2022-jp?B?RllTUjA3MlpraUdoNlV5VnlnUU50MVpIZC9qd0lOQVE5KzhUdVlhTjRo?=
 =?iso-2022-jp?B?TXJRZm05Nm96QWxteTRuTTdvNUYzeWFFaFhFdkUwM2FSOGRRZFFIaUJY?=
 =?iso-2022-jp?B?MEJGcnd2NVdGUXlnM2dIT0FsYlVpbTVndDNvMHBFUGxldm93SkZzZzVo?=
 =?iso-2022-jp?B?TnU0SGpCQWdWdXpUZUYreFR3NUx5eHBET1kyZ0FSUU1UZmg5eUt5dWRN?=
 =?iso-2022-jp?B?cXBzbXBPd3d3VnE2WWh3bTllWXczMjUrTjhkbThlTCtGRG9QUWFPMXhy?=
 =?iso-2022-jp?B?SnZxK01MckRQS0ZRUkcyeVBtU3JTR2xtY2REZ2VyYkFnY0RMT3RXRDcw?=
 =?iso-2022-jp?B?QXVXODAwZXFNcVJjNFdQcFdBSkwvWThvRzlBZG5wc3ordS9qVTJtQ2Iy?=
 =?iso-2022-jp?B?ajE0RHFFc1Y5enJmWC9saE1oT1J0dDZaTy9WYlkzTExndW4vbGRVZzhz?=
 =?iso-2022-jp?B?SjNNKy8wSVRrajkzK043MXdOUmxPT09lZG9nYzc5Wm5IZUp5SXlRakFi?=
 =?iso-2022-jp?B?QmVvTHBvL2hDdlhSeXJOMDZnN3BBYm5YeVdDT09PVXNCODc4elFucGxw?=
 =?iso-2022-jp?B?RkpBUThHaWVGTEdDWXUxZG51ZlNPM1BtUzFDcFprVDhubjBLNTdjcWRW?=
 =?iso-2022-jp?B?bEllall2bjRSU1lqNkhDcEFnVytwaGtqdjQ1S2J0RTdYVmI1M0htWUgw?=
 =?iso-2022-jp?B?eDJsOXlMcFFQbFBFaSt6M1ZVbDJ5UFFpSTY4Y3MramlqdExaODArbWVu?=
 =?iso-2022-jp?B?OFNaM2VDbTdnVFZ0Vm1MMG9nSTRDWXROSzNtT2pxOWxlaWtVSmtXL2lM?=
 =?iso-2022-jp?B?T1ZLTk41WFU1a0NRaXlFclE3YUdHaWtsVEo3UEVvVU82ZzVpQ3hHVXBM?=
 =?iso-2022-jp?B?NFV2RlFaMDJNVStDblBUa1RYQld3WUc3ZkNsSFgrRS9IZHpmcDgyZ1N4?=
 =?iso-2022-jp?B?U0pKZnhFcFpmV2NmTzlSZHNSeW9UOURtZ2FRTWhDdmxVZHZPT0oxTS9R?=
 =?iso-2022-jp?B?aWI4NjVNWXlYNDJhbnc0RFZSQUlPVzZYeHc2MzdiWkhCRkxtclFsSzhX?=
 =?iso-2022-jp?B?akZIMFZ6RnN2R2JSOWh6SzNMZ2s0YnRreDZYbXlKY014ODN1d0RWVk1i?=
 =?iso-2022-jp?B?ZDg1d3ZRb1Rwemx5ejVacXBEaU53ZU13RERmdFJ2TmFJamFqWTAzM05R?=
 =?iso-2022-jp?B?UUdhMw==?=
Content-Type: text/plain; charset="iso-2022-jp"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR18MB4474.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e0cd3313-f3a5-46ef-7919-08dc73f7e27b
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 May 2024 09:26:07.3572
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HH9hoaECFRhjfMyqA03e0v+JCi3STk8lyrzSwokA7KidSUs6sqUZj9fxklYBWUMXQtE6VMpIFE2GXlH2WtD5ow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR18MB4112
X-Proofpoint-ORIG-GUID: KJ0wcnlCwoJj8kpEYzKdY9BnlYQFZGwV
X-Proofpoint-GUID: KJ0wcnlCwoJj8kpEYzKdY9BnlYQFZGwV
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-14_04,2024-05-10_02,2023-05-22_02


> > -----Original Message-----
> > From: Hariprasad Kelam <hkelam@marvell.com>
> > Sent: 2024=1B$BG/=1B(B5=1B$B7n=1B(B13=1B$BF|=1B(B 17:27
> > To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Shenwei
> Wang
> > <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > Subject: [PATCH net] net: fec: avoid lock evasion when reading
> > pps_enable
> >
> > See inline,
> >
> > > The assignment of pps_enable is protected by tmreg_lock, but the
> > > read operation of pps_enable is not. So the Coverity tool reports a
> > > lock evasion warning which may cause data race to occur when running
> > > in a multithread environment. Although this issue is almost
> > > impossible to occur, we'd better fix it, at least it seems more
> > > logically reasonable, and it also prevents Coverity from continuing t=
o issue
> warnings.
> > >
> > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp
> > > clock")
> > > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > > ---
> > >  drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> > > b/drivers/net/ethernet/freescale/fec_ptp.c
> > > index 181d9bfbee22..8d37274a3fb0 100644
> > > --- a/drivers/net/ethernet/freescale/fec_ptp.c
> > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct
> > > fec_enet_private *fep, uint enable)
> > >  	struct timespec64 ts;
> > >  	u64 ns;
> > >
> > > -	if (fep->pps_enable =3D=3D enable)
> > > -		return 0;
> > > -
> > >  	fep->pps_channel =3D DEFAULT_PPS_CHANNEL;
> > >  	fep->reload_period =3D PPS_OUPUT_RELOAD_PERIOD;
> > >
> > >  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > >
> > > +	if (fep->pps_enable =3D=3D enable) {
> >
> > Can we atomic_set/get instead of spin_lock here.
> >
> I'm afraid that cannot eliminate the lock evasion warning, because it's s=
till
> possible that multithreads take the false branch of "if (fep->pps_enable =
=3D=3D
> enable)" before pps_enable is updated.
>=20
> Since  in fec_ptp_enable_pps(), value of pps_enable is checked before ent=
ering the actual code changes,
  Better approach is to use atomic_read/write. This is not only for reading=
 but for assigning the values as well.
  This way covertity wont complain.


> > Thanks,
> > Hariprasad k
> > > +		spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> > > +		return 0;
> > > +	}
> > > +
> > >  	if (enable) {
> > >  		/* clear capture or output compare interrupt status if have.
> > >  		 */
> > > --
> > > 2.34.1
> > >