> -----Original Message----- > From: Jingoo Han [mailto:jingoohan1@xxxxxxxxx] > Sent: Friday, August 21, 2015 12:08 PM > 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 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. Yes > > > 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. Ok no probs, thanks for reviewing. Gab > > 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
