diff mbox series

PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq()

Message ID 3f9f779c-a32f-4925-9ff9-a706861d3357@moroto.mountain
State New
Headers
Series PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq() |

Commit Message

Dan Carpenter Jan. 17, 2024, 6:32 p.m. UTC 
  The "msg_addr" variable is u64.  However, the "tbl_offset" is an unsigned
int.  This means that when the code does

	msg_addr &= ~aligned_offset;

it will unintentionally zero out the high 32 bits.  Declare "tbl_offset"
as a u64 to address this bug.

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).

 drivers/pci/controller/dwc/pcie-designware-ep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Niklas Cassel Jan. 17, 2024, 9:21 p.m. UTC | #1 
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
> 
> 	msg_addr &= ~aligned_offset;
> 
> 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.
> 
> 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).
> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/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 = 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 = ((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 = ((u64)msg_addr_upper) << 32 | msg_addr_lower;
msg_addr &= ~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
Dan Carpenter Jan. 19, 2024, 8:25 a.m. UTC | #2 
On Wed, Jan 17, 2024 at 09:21:41PM +0000, Niklas Cassel wrote:
> 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
> > 
> > 	msg_addr &= ~aligned_offset;
> > 
> > it will unintentionally zero out the high 32 bits.  Declare "tbl_offset"
> 
> Here you also write tbl_offset.
> 

That's so weird...  I can't imagine how that happened.  Do you think it
could be a Welsh mice situation where forest creatures are changing my
work when I'm away from my desk?  https://www.youtube.com/shorts/h8gkIbtaaek

Fixed in v2.  Thanks!

regards,
dan carpenter
Niklas Cassel Jan. 19, 2024, 12:06 p.m. UTC | #3 
On Fri, Jan 19, 2024 at 11:25:51AM +0300, Dan Carpenter wrote:
> On Wed, Jan 17, 2024 at 09:21:41PM +0000, Niklas Cassel wrote:
> > 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
> > > 
> > > 	msg_addr &= ~aligned_offset;
> > > 
> > > it will unintentionally zero out the high 32 bits.  Declare "tbl_offset"
> > 
> > Here you also write tbl_offset.
> > 
> 
> That's so weird...  I can't imagine how that happened.  Do you think it
> could be a Welsh mice situation where forest creatures are changing my
> work when I'm away from my desk?  https://www.youtube.com/shorts/h8gkIbtaaek

Yes, that it the most likely scenario :)

In fact, I think that is what happened with my original patch too...

Because while the C-standards says:

"""
6.5.3.3 Unary arithmetic operators
The result of the unary - operator is the negative of its (promoted) operand. The integer
promotions are performed on the operand, and the result has the promoted type.
"""


Of course, I also remember that implicit integer promotions are only up to
"int" or "unsigned int".

Because of course it is fine to convert types smaller than int implicitly...
but bigger than int? No way! :)


#include <stdio.h>
#include <stdint.h>

void main()
{
        uint16_t val_16 = 0xffff;
        uint8_t mask_8 = 0xf0;

        uint32_t val_32 = 0xffffffff;
        uint16_t mask_16 = 0x00f0;

        uint64_t val_64 = 0xffffffffffffffff;
        uint32_t mask_32 = 0x000000f0;

        uint16_t res_16 = val_16 & ~mask_8;
        uint32_t res_32 = val_32 & ~mask_16;
        uint64_t res_64 = val_64 & ~mask_32;

        printf("16: res: %#llx val: %#llx mask: %#llx\n", res_16, val_16, mask_8);
        printf("32: res: %#llx val: %#llx mask: %#llx\n", res_32, val_32, mask_16);
        printf("64: res: %#llx val: %#llx mask: %#llx\n", res_64, val_64, mask_32);
}

output:
16: res: 0xff0f val: 0xffff mask: 0xf0
32: res: 0xffffff0f val: 0xffffffff mask: 0xf0
64: res: 0xffffff0f val: 0xffffffffffffffff mask: 0xf0

(Silly me for not also reading 6.3.1.1...)


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/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 = ep->epc;
 	u32 reg, msg_data, vec_ctrl;
-	unsigned int aligned_offset;
+	u64 aligned_offset;
 	u32 tbl_offset;
 	u64 msg_addr;
 	int ret;