| Message ID | 3f9f779c-a32f-4925-9ff9-a706861d3357@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq() | expand |
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
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
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 --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;
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(-)