From:   Pawel Laszczak <pawell@cadence.com>
To:     Peter Chen <peter.chen@kernel.org>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jianhe@ambarella.com" <jianhe@ambarella.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: RE: [PATCH] usb: cdnsp: Fix lack of
 spin_lock_irqsave/spin_lock_restore
Thread-Topic: [PATCH] usb: cdnsp: Fix lack of
 spin_lock_irqsave/spin_lock_restore
Thread-Index: AQHX8BvTZRkkqcJmf0Wg5yGRfvbOhqwwasUAgAAA2GA=
Date:   Mon, 13 Dec 2021 14:24:02 +0000
Message-ID: <BYAPR07MB53811E2C7CB05B9DEACF7A8CDD749@BYAPR07MB5381.namprd07.prod.outlook.com>
References: <20211213122001.47370-1-pawell@gli-login.cadence.com>
 <20211213132946.GD5346@Peter>
In-Reply-To: <20211213132946.GD5346@Peter>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?mfq7SmoebjJhhtw/JAoDFVBBvGCf+HEfIAcwHoDFX4bh5n0pRjhGthBmFRTM?=
 =?us-ascii?Q?f2fk/adnn12WP6N3svEAoBAHv5oKt4bvBr3E50faZ6G1LxorJ86YuaqMjPsm?=
 =?us-ascii?Q?kr1mmLieLfW7i3jcvQJ4HBlvpxhwrYiyxfbZE7GZWH/+LrGas42fp6dIfYR+?=
 =?us-ascii?Q?h3KJfP1k4keRRP7GosVmpCNjkn3ea0N+TYeNSZSLaa4GyggEUaPp2rKF7zDN?=
 =?us-ascii?Q?lyiqcD1SzeJH09/oTk/Z38+2/O1iSazkTFH5+BrORd11oaL2K63rXK7OF5Cd?=
 =?us-ascii?Q?b/6gbtWer+jKUTiQqENIsWZTD20Mi1W+yvznVn1jHVf5xjGkUcVzMCM/k5qt?=
 =?us-ascii?Q?3FEicDC2EW1NrbjSt1TZ/ClP3TsIzLkXxlYnSRTgKQkyDhtUKTlhkS14mHBN?=
 =?us-ascii?Q?u+wvov5jbiy8NfJY1yGZ6ZafbEdhoVGMdxO+Dkb4EEhzULePAy6593HsatyG?=
 =?us-ascii?Q?nZMIUNLj7xRcTUnhE17OSHpBHmvo6DZBT3OUli0QZKh/QNMY2tlY3Or1K9EV?=
 =?us-ascii?Q?adLDRpUs5OiEES/vXa7DuJP1c2cn0pbvyZK8diTeXGss33BEsb4A+dungFf2?=
 =?us-ascii?Q?89QF6KYEqdbQ58X0dFQSB8UN9jFn0YoTSDkhdeLWjrgKJdPwC8yjdlp/JBYK?=
 =?us-ascii?Q?p5voM0ZBnVQLVJgMOY5C2G9dFBmQI1mIhBjgcuhg74GzBhJP+V3+pDYJ1PJp?=
 =?us-ascii?Q?Jcq6jmhG0+nRNC9Cgp69BCkWT+6e3m1GF9VrNLKjojT3atxUafcit46OoDVM?=
 =?us-ascii?Q?teUWiMKEpkzOjpsFHR5pZOsX4tM6E9sMRbTfbyOPUSFh1yXLBwOdkfdnmiq3?=
 =?us-ascii?Q?IQm2ZGcdGJz1E/wYDS97pkUcsO0EL0nw48Oij64oaqNzxehkovdatICWju27?=
 =?us-ascii?Q?MGJJZ6AI0k+UZ4PrxNrpCgOMQ03rWZat/E9ljSEoS+JejI0hGp9DVkeIIFb5?=
 =?us-ascii?Q?VtCDghf9vnZX/RLGaGbriaI20Y45hMCDVVfSA2EkCU3DuRRip8hKCdFDpzEV?=
 =?us-ascii?Q?ty8V0egdNpgx1P2RD6y1AVqG2H3LG41ov6Lau6vOB/YbBjriNinCQ3Vsfldq?=
 =?us-ascii?Q?kYTf7tBr9iqmYHfxV9GB7Ta4zCZSN68ga/g1qjZowI9qZqlTNv46MxZms3jk?=
 =?us-ascii?Q?opiQtbNaemsCaGLFnqxQ5m92naplKH+9QWdhjhkdLGIi8UPt8+Y9ZCWk6Jj1?=
 =?us-ascii?Q?pyTVjBBqggxi4VvB3nSW7GfGawRq5HMMC7ZJkCrniNWENXLLXAHkQW0mQXC9?=
 =?us-ascii?Q?fPMuhWe1OqzETtYf8iwB7EFgPIyH3SwXFmcnoXFcYUElCeF56Bj8Gm+PgcMc?=
 =?us-ascii?Q?FgmXhwDBDmi81W4eAnDdlmQ8kfWbC9kxxzdqyvPip+Gu3bDOL8eadKZ4jVpE?=
 =?us-ascii?Q?1dG8NJXN1IVuY/JXqgda8v0y6aefRMUQLZFGz000KA4vlQOM19ETBenMTEy4?=
 =?us-ascii?Q?jukTSFvurAlc0QSox45dNUSfEYEt8AZQgrEl6+uW/MNBLidMvyWKj37aY+po?=
 =?us-ascii?Q?9WJCh+S6qkRU8rj6sPH7dBKS/cQPJBCcPSct/J2sRTZXXoPYVwVcJwN/Hhrm?=
 =?us-ascii?Q?LjIxuF+0mp1J5+NMtuuJC0FJS+HfC8Z1PK9yRc8x9zHYmi4y6PQfDifKPvRh?=
 =?us-ascii?Q?HKxDZwtt0OyeZdsr4LjnsefDLNrbvWmYsN4hq7mOeD0x7/qiWbN1U1Z3OmwA?=
 =?us-ascii?Q?D6exIQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: cadence.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BYAPR07MB5381.namprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 928101ba-107d-44b7-b010-08d9be443667
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Dec 2021 14:24:02.7715
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: kUpRxiTm2Y/7p1GAP1rnl5BrWLc0AFCeRq/Kxa3EjY6OsNN2lfeMxEjxwu5tm1x8oiY8f7BaDPdURYfftwuvVI/Yhfkji7p65UBA+VWZ+3A=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR07MB5127
X-Proofpoint-ORIG-GUID: fzK3qUofMedcIoITTVkhoPJjKn2HgdV-
X-Proofpoint-GUID: fzK3qUofMedcIoITTVkhoPJjKn2HgdV-
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2021-12-13_06,2021-12-13_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 spamscore=0
 suspectscore=0 malwarescore=0 adultscore=0 clxscore=1015 impostorscore=0
 lowpriorityscore=0 mlxscore=0 mlxlogscore=919 phishscore=0 bulkscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2112130092
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>
>On 21-12-13 13:20:01, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@cadence.com>
>>
>> Patch puts content of cdnsp_gadget_pullup function inside
>> spin_lock_irqsave and spin_lock_restore section.
>> This construction is required here to keep the data consistency,
>> otherwise some data can be changed e.g. from interrupt context.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP =
DRD Driver")
>> Reported-by: Ken (Jian) He <jianhe@ambarella.com>
>> cc: <stable@vger.kernel.org>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-gadget.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-=
gadget.c
>> index f6d231760a6a..d0c040556984 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -1544,8 +1544,10 @@ static int cdnsp_gadget_pullup(struct usb_gadget =
*gadget, int is_on)
>>  {
>>  	struct cdnsp_device *pdev =3D gadget_to_cdnsp(gadget);
>>  	struct cdns *cdns =3D dev_get_drvdata(pdev->dev);
>> +	unsigned long flags;
>>
>>  	trace_cdnsp_pullup(is_on);
>> +	spin_lock_irqsave(&pdev->lock, flags);
>
>If the interrupt bottom half is pending, the consistent issue may still
>exist, you may let the bottom half has finished first, eg: calling
>disable_irq before spin_lock_irqsave.
>
>Peter
>>

But bottom half procedure is also protected by spin lock, so it will be wai=
ting for completion
cdnsp_gadget_pullup and vice versa.

I think you means the case when driver in bottom half function release the =
spin lock before calling some API function.
and in this moment the pullup function starts to be handled.=20
I didn't detect such issue, but theoretically it is possible.

Let me test option with disable_irq before spin_lock_irqsave.

>>  	if (!is_on) {
>>  		cdnsp_reset_device(pdev);
>> @@ -1553,6 +1555,9 @@ static int cdnsp_gadget_pullup(struct usb_gadget *=
gadget, int is_on)
>>  	} else {
>>  		cdns_set_vbus(cdns);
>>  	}
>> +
>> +	spin_unlock_irqrestore(&pdev->lock, flags);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.25.1
>>
>

--

Thanks,
Pawel Laszczak