Received-SPF: pass (google.com: domain of linux-kernel+bounces-29457-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
CC: Jingoo Han <jingoohan1@gmail.com>, Gustavo Pimentel
	<gustavo.pimentel@synopsys.com>, Manivannan Sadhasivam
	<manivannan.sadhasivam@linaro.org>, Lorenzo Pieralisi
	<lpieralisi@kernel.org>, =?iso-8859-2?Q?Krzysztof_Wilczy=F1ski?=
	<kw@linux.com>, Rob Herring <robh@kernel.org>, Bjorn Helgaas
	<bhelgaas@google.com>, "linux-pci@vger.kernel.org"
	<linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "kernel-janitors@vger.kernel.org"
	<kernel-janitors@vger.kernel.org>
Subject: Re: [PATCH] PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq()
Thread-Topic: [PATCH] PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq()
Thread-Index: AQHaSXODJQL0XZUgYEa//+ZWpXISHbDeg2qA
Date: Wed, 17 Jan 2024 21:21:41 +0000
Message-ID: <ZahE455neE3wPnHA@x1-carbon>
References: <3f9f779c-a32f-4925-9ff9-a706861d3357@moroto.mountain>
In-Reply-To: <3f9f779c-a32f-4925-9ff9-a706861d3357@moroto.mountain>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?iso-8859-2?Q?ZmdZCxKuzbbSdGKhKc9hX+nhkqYQyMupwHe6PyvOVwxyFtacWmjDnh5AlO?=
 =?iso-8859-2?Q?9vXJs7QS8p+bF0/iZUYeBpKuffN2vLN/r3c66zSB9GK+Z2O6fNYFnDCxMW?=
 =?iso-8859-2?Q?jPCZjqxHUF7IUx3Y98LGl/YBchOfMc1b6n5fiT/a22SMpOIEILNf4qV7Qh?=
 =?iso-8859-2?Q?G8AVsrERWr1aS2H6yOJEFCLbZ0kuobk+XsLMzqDhDxRc4LXRFyBY+rgzuo?=
 =?iso-8859-2?Q?2jIFgxdzcVT1pAjolLo+7Gwra+obS3xVzsAGFS0howgNFKsSPGpS2QJKzP?=
 =?iso-8859-2?Q?MxyvhRUc+Bq9+900Y5zfnFUfNMjgLwtGNMtynWek0gJWPws4xhqdTd9BQ3?=
 =?iso-8859-2?Q?/alnXdEYd6p5CiUrUo8Xcyx6e8XnWHeXDB21MVYUCp2HkmkUNM0XliIi8S?=
 =?iso-8859-2?Q?ZfSDrR8+KaFPkn0p51pReKSZaqKNbxG6Q4W6Y/g8G8PD8uLWQrUcrhtq7X?=
 =?iso-8859-2?Q?wJaweqsaecvlnP/zRkR4HwsW8SprDvuGInoKkAskDvPUUeqmqCh4MjyHvO?=
 =?iso-8859-2?Q?ejsf3RndN7eLbrNNkUs0E4RzPnJzGZZThQVauW5n63jaRgJySh6CoVctLY?=
 =?iso-8859-2?Q?6OEQgC5UrDLP68my/UfcYu9ZTVmkuTGKLAB+3N1zr/KRgeoIbC+toPx6BA?=
 =?iso-8859-2?Q?DhFta+f4o6mkY3Xa4SbIbEPGAQ1K+A6foU12VANeypeTuakZO1X2unn/o3?=
 =?iso-8859-2?Q?4FRPr6WcnAwbib42hpPx6XQla3pqsxGuSn4FSrVPrudvBC25j8hzh+vdR8?=
 =?iso-8859-2?Q?4NLSwBIsJvnENkWXhjdaMMPr0YlZYvCBPdnHw/jPumEseOMxL3zGHpKEmK?=
 =?iso-8859-2?Q?QjmvrUHtxeZr38gwFDYbE0UFNqj5LgAk/OX+37zqqn/fCXLfWtFnqgfvYA?=
 =?iso-8859-2?Q?wGFsB3VdqNeZaXKM7jQQJyHgDTlIzqdqjLqYnsK/dQDX9SQygtVGzTD7i5?=
 =?iso-8859-2?Q?tTxytSWH1MLqk2l53OlcLloFP3qNjfGYIaOsDLC1JBY4bheHAyywBmLh4G?=
 =?iso-8859-2?Q?hZ1YhgPogPuLa33ggMnHKOiheIy9OiItlS1lGd0Z50tPb9RrMaJ+HpL/Sz?=
 =?iso-8859-2?Q?KXoDbxPJXDur6LdlbeXRX8L2x3T6dzMS58yrLnbU7gsMm4rOpd3ShtrBtV?=
 =?iso-8859-2?Q?pu03vFoEd1cICUmihAhENnnBgAiz8xvhDZmkk1H9tmdZN0hSwK7/U2Z7lV?=
 =?iso-8859-2?Q?h6xSGk9gEJf2xQyCuXzpXZ5ub5Ne0V6mnS+g6OtjiZYyBjz/5HuZ6pFbnn?=
 =?iso-8859-2?Q?C8KOQ0DVTEIABwDgG8hVgwO4IVo3lyVmQWmaOygiyony513O5fLyldkWUw?=
 =?iso-8859-2?Q?LzXUAGC8O7JnQxa5ZwtovH2tjh50cufn8BonqYiV6Wmf/UNsgQ/8YOQ/dn?=
 =?iso-8859-2?Q?CGmgYKHK3Su6JyQsUDqmFBcODXQwyEN7pTcBVLBgj1y2tkuamHRs2g65WG?=
 =?iso-8859-2?Q?VtE+h4Eh3ajliEWU/m21S7kXWNZzXl6ZLMgggMeCRGIpB7u6MqSSLR0pxE?=
 =?iso-8859-2?Q?3M093WdXBIsJyMXOi0Pm44rfoNmVzAPRWXc8L4LueHJ4AgYlEQF2aYIKZP?=
 =?iso-8859-2?Q?QuqS1R7FwnMT7+Ozg3cnU/9lF729xTy0gVIpDXYUxwVeVHjDwTG4sQFL5Q?=
 =?iso-8859-2?Q?jxRzsr40Fq/fKtTgo7sOcJlkhxih+MLhiH?=
Content-Type: text/plain; charset="iso-8859-2"
Content-ID: <A99151DF02266F4F91AD13020260F28D@namprd04.prod.outlook.com>
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-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0:
	8MqpvQ8OMDmQ6kHArU4x1H2DR0sKr8AAWDZ2uBAgDPtnEXVUeO8doBT4lFJfVnl2bs7vbEO6M3yDSDw/gyPryGidZrHt213T46xqQxAHyTHHYwum28qVyVDKPYzh9wqpguLCYf1jqNSZ/CkICMxiRu6imGBIBoju7msP9AK7h1XPAL4v4VIko1S7ewyo2NMeAdvDvPaz0Z2Grjk/poxiyHlLg78+vh6e+jBqEIBha6vUOJqqY608Y1Fddk/Z1SAVT9w0jKbjd1UDxGh5EJGTaP6g/Qo9qe5JcBvtaMGstssRV9nDKfJ24OxlAHDFLKXVKNTF1tPAF1hs47xl2C+8e5bKVUN+Po2gX+kIncIrJYqTHrfvrAPDG1YjJwH2w0CZCNnQN6vII/tkuJKzkhq9QXKrbWzE2L5s8rM6v0gSEVKow9cTT2KtUpEfJPsLCRo2u9V8E6iSxCKVZfKeqQGX49FgCIKj0hUOiz9UHb6GTtD1NlMX7iw9ExyxIRXJKq/wlHLQqvM9OU1P7OCSWGzcV9yCQSfDkED4kabGSqhO6ewEIwTQO2gASzrrzlzJec9f8TDui76J+gba/8Yw3ee2p8IKH0Q1CN+hoHMdT3v8U9st1D8tixBIQpElKOkSL4Wg
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MWHPR04MB1040.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: afc15497-e80b-4d1b-ced3-08dc17a24c45
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 21:21:41.1445
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: TfAWL0gFL1UTFQ7W73Qs8t/wBuCMYSWB7tTciJi7ukxs6O4vGvb0QqyEAjKw7kBOCK9vy1jNjBJzUo/hAz+Vaw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR04MB7155

Hello Dan,

On Wed, Jan 17, 2024 at 09:32:08PM +0300, Dan Carpenter wrote:
> The "msg_addr" variable is u64.  However, the "tbl_offset" is an unsigned

Here you write tbl_offset.

> int.  This means that when the code does
>=20
> 	msg_addr &=3D ~aligned_offset;
>=20
> it will unintentionally zero out the high 32 bits.  Declare "tbl_offset"

Here you also write tbl_offset.

> as a u64 to address this bug.
>=20
> Fixes: 2217fffcd63f ("PCI: dwc: endpoint: Fix dw_pcie_ep_raise_msix_irq()=
 alignment support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> From static analysis (not tested).
>=20
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pc=
i/controller/dwc/pcie-designware-ep.c
> index 5befed2dc02b..2b6607c23541 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -525,7 +525,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, =
u8 func_no,
>  	struct dw_pcie_ep_func *ep_func;
>  	struct pci_epc *epc =3D ep->epc;
>  	u32 reg, msg_data, vec_ctrl;
> -	unsigned int aligned_offset;
> +	u64 aligned_offset;

Yet here you change actually change aligned_offset.

Since msg_addr is u64, aligned_offset should also be u64.
Sorry that I missed this.
I followed the pattern of dw_pcie_ep_raise_msi_irq().

I've tested my original patch, but the MSI address must have been in the
lower 32-bits.
Thank you for the fix!


If you modify dw_pcie_ep_raise_msix_irq(), perhaps we
should also modify dw_pcie_ep_raise_msi_irq(),
as it also has aligned_offset defined as "unsigned int"
and msg_addr as "u64".


Looking more carefully at dw_pcie_ep_raise_msi_irq(), it has:

u64 msg_addr;
u32 msg_addr_lower, msg_addr_upper;

and does:

	msg_addr =3D ((u64)msg_addr_upper) << 32 |
			(msg_addr_lower & ~aligned_offset);

So there is no problem there as that operation is performed only on
msg_addr_lower, which is u32.

However, perhaps we should also modify dw_pcie_ep_raise_msi_irq(),
so that "aligned_offset" is u64 instead of "unsigned int",
so that it also matches the msi_data.

That way dw_pcie_ep_raise_msi_irq() could instead look like this:
msg_addr =3D ((u64)msg_addr_upper) << 32 | msg_addr_lower;
msg_addr &=3D ~aligned_offset;

which is slightly more readable IMO, and will ensure that
dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq()
are more similar. But I will leave that decision up to you.


Kind regards,
Niklas=