On 2015. 8. 21., at PM 7:29, Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx> wrote: > >> -----Original Message----- >> From: Jingoo Han [mailto:jingoohan1@xxxxxxxxx] >> Sent: Friday, August 21, 2015 10:25 AM >> To: Gabriele Paoloni >> Cc: Wangzhou (B); <bhelgaas@xxxxxxxxxx>; <linux-pci@xxxxxxxxxxxxxxx>; >> <pratyush.anand@xxxxxxxxx>; Yuanzhichang; Zhudacai; zhangjukuo; >> qiuzhenfa; Liguozhu (Kenneth); jingoohan1@xxxxxxxxx >> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write >> >> On 2015. 8. 21., at PM 5:58, Gabriele Paoloni >> <gabriele.paoloni@xxxxxxxxxx> wrote: >>> >>> Hi Jingoo >>> >>>> -----Original Message----- >>>> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci- >>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Jingoo Han >>>> Sent: Friday, August 21, 2015 5:48 AM >>>> To: Wangzhou (B) >>>> Cc: Gabriele Paoloni; <bhelgaas@xxxxxxxxxx>; <linux- >>>> pci@xxxxxxxxxxxxxxx>; <pratyush.anand@xxxxxxxxx>; Yuanzhichang; >>>> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth); >>>> jingoohan1@xxxxxxxxx >>>> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write >>>> >>>>> On 2015. 8. 21., at AM 10:51, Zhou Wang <wangzhou1@xxxxxxxxxxxxx> >> wrote: >>>>> >>>>>> On 2015/8/20 21:58, Gabriele Paoloni wrote: >>>>>> From: gabriele paoloni <gabriele.paoloni@xxxxxxxxxx> >>>>>> >>>>>> Currently in dw_pcie_cfg_write() if the input size is 2 bytes >>>>>> the address offset is wrongly calculated as we mask also bit0 >>>>>> of "where" input parameter. Instead we should mask all bits of >>>>>> "where" except bit0 and bit1. >>>>>> >>>>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx> >>>>>> --- >>>>>> drivers/pci/host/pcie-designware.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/pci/host/pcie-designware.c >>>> b/drivers/pci/host/pcie-designware.c >>>>>> index 69486be..a27f536 100644 >>>>>> --- a/drivers/pci/host/pcie-designware.c >>>>>> +++ b/drivers/pci/host/pcie-designware.c >>>>>> @@ -99,7 +99,7 @@ int dw_pcie_cfg_write(void __iomem *addr, int >>>> where, int size, u32 val) >>>>>> if (size == 4) >>>>>> writel(val, addr); >>>>>> else if (size == 2) >>>>>> - writew(val, addr + (where & 2)); >>>>>> + writew(val, addr + (where & 3)); >>>>>> else if (size == 1) >>>>>> writeb(val, addr + (where & 3)); >>>>>> else >>>>> >>>>> Hi Jingoo, >>>>> >>>>> Is there some special consideration? If we miss something, please >> let >>>> us know. >>>> >>>> This patch is unnecessary. >>>> >>>> In the case of 'size == 2', the writew() in dw_pcie_cfg_write() >> should >>>> handle the following values of 'where'. >>>> >>>> h00 b0000 >>>> h02 b0010 >>>> >>>> Thus, there is no need to keep 0th bit. >>> >>> What if the last bits of "where" are h01(b0001), then the current >> implementation will clear the last bit...? >> >> Yep. >> >>> Do you think it is impossible to have a scenario where: "where = >> hxxxxxx01" and "size = 2"? >> >> Is there any function using "where = h01" and "size =2"? >> It might be wrong usage. > > Please correct me if I am wrong... > For example if a designware based controller needs to modify > PCI_CLASS_PROG, then it will end up writing PCI_REVISION_ID. You mean PCI_CLASS_PROG (0x09) and PCI_CLASS_DEVICE(0x10) with size 2. > Now this is maybe not happening now, but, since it is a quick fix and > since dw_pcie_cfg_write() is a base access function, I just thought > better to have it fixed now to avoid any future bug... I don't want to support these tricky and uncommon cases. Also, when writew() is called, "STRH" is used in ARM SoCs. In this case, unaligned access happens, when 0th bit of addr is 1. writew() is more beneficial than writel() when accessing address with 2 bytes. Sorry, I don't find any benefits to support the unaligned access. Best regards, Jingoo Han > However since you re the maintainer it is up to you... > >> >> Best regards, >> Jingoo Han >> >>>> >>>> One more thing, is there any reason to urge me to review this patch >>>> within a few hours? >>>> Please wait for reviews for enough hours. >>>> >>>> Maybe, your company looks to support you and other engineers for >>>> mainline kernel. >>>> However, I am not supported at all, so it is not easy to review the >>>> patches quickly. >>> >>> Jingoo I am sorry for this. I am sure Zhou Wang did not mean to push >> you. >>> >>> I agree that this patch is low prio...for us I think is much more >> important >>> to have "PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05" >> patchset >>> pushed in mainline. >>> >>>> Best regards, >>>> Jingoo Han >>>> >>>>> Thanks, >>>>> Zhou >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" >> in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
