PCI: designware: fix dw_pcie_cfg_write

Commit Message

Gabriele Paoloni Aug. 20, 2015, 1:58 p.m. UTC

From: gabriele paoloni <[e-mail]>

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 <[e-mail]>
---
 drivers/pci/host/pcie-designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Zhou Wang Aug. 21, 2015, 1:51 a.m. UTC | #1

On 2015/8/20 21:58, Gabriele Paoloni wrote:
> From: gabriele paoloni <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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.

Thanks,
Zhou

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Han Jingoo Aug. 21, 2015, 4:48 a.m. UTC | #2

On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]> wrote:
> 
>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
>> From: gabriele paoloni <[e-mail]>
>> 
>> 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 <[e-mail]>
>> ---
>> 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.

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.

Best regards,
Jingoo Han

> Thanks,
> Zhou
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhou Wang Aug. 21, 2015, 5:16 a.m. UTC | #3

On 2015/8/21 12:48, Jingoo Han wrote:
> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]> wrote:
>>
>>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
>>> From: gabriele paoloni <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>> 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.
> 
> 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.

Honestly speaking, I didn't mean to push you to review it :)

I thought maybe there was some hardware issues involved in above patch,
just asking a question about it. Anyway, maybe I should have waited for your reply.

Many thanks,
Zhou

> 
> 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.
>
> Best regards,
> Jingoo Han
> 
>> Thanks,
>> Zhou
>>
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni Aug. 21, 2015, 8:58 a.m. UTC | #4

Hi Jingoo 

> -----Original Message-----
> From: [e-mail] [mailto:linux-pci-
> [e-mail]] On Behalf Of Jingoo Han
> Sent: Friday, August 21, 2015 5:48 AM
> To: Wangzhou (B)
> Cc: Gabriele Paoloni; <[e-mail]>; <linux-
> [e-mail]>; <[e-mail]>; Yuanzhichang;
> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
> [e-mail]
> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> 
> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]> wrote:
> >
> >> On 2015/8/20 21:58, Gabriele Paoloni wrote:
> >> From: gabriele paoloni <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> ---
> >> 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...? 
Do you think it is impossible to have a scenario where: "where = hxxxxxx01" and  "size = 2"? 


> 
> 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Han Jingoo Aug. 21, 2015, 9:25 a.m. UTC | #5

On 2015. 8. 21., at PM 5:58, Gabriele Paoloni <[e-mail]> wrote:
> 
> Hi Jingoo 
> 
>> -----Original Message-----
>> From: [e-mail] [mailto:linux-pci-
>> [e-mail]] On Behalf Of Jingoo Han
>> Sent: Friday, August 21, 2015 5:48 AM
>> To: Wangzhou (B)
>> Cc: Gabriele Paoloni; <[e-mail]>; <linux-
>> [e-mail]>; <[e-mail]>; Yuanzhichang;
>> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
>> [e-mail]
>> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
>> 
>>> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]> wrote:
>>> 
>>>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
>>>> From: gabriele paoloni <[e-mail]>
>>>> 
>>>> 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 <[e-mail]>
>>>> ---
>>>> 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.

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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni Aug. 21, 2015, 10:29 a.m. UTC | #6

> -----Original Message-----
> From: Jingoo Han [mailto:[e-mail]]
> Sent: Friday, August 21, 2015 10:25 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); <[e-mail]>; <[e-mail]>;
> <[e-mail]>; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth); [e-mail]
> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> 
> On 2015. 8. 21., at PM 5:58, Gabriele Paoloni
> <[e-mail]> wrote:
> >
> > Hi Jingoo
> >
> >> -----Original Message-----
> >> From: [e-mail] [mailto:linux-pci-
> >> [e-mail]] On Behalf Of Jingoo Han
> >> Sent: Friday, August 21, 2015 5:48 AM
> >> To: Wangzhou (B)
> >> Cc: Gabriele Paoloni; <[e-mail]>; <linux-
> >> [e-mail]>; <[e-mail]>; Yuanzhichang;
> >> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
> >> [e-mail]
> >> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> >>
> >>> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]>
> wrote:
> >>>
> >>>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
> >>>> From: gabriele paoloni <[e-mail]>
> >>>>
> >>>> 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 <[e-mail]>
> >>>> ---
> >>>> 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.

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

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 [e-mail]
> >> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Han Jingoo Aug. 21, 2015, 11:07 a.m. UTC | #7

On 2015. 8. 21., at PM 7:29, Gabriele Paoloni <[e-mail]> wrote:
> 
>> -----Original Message-----
>> From: Jingoo Han [mailto:[e-mail]]
>> Sent: Friday, August 21, 2015 10:25 AM
>> To: Gabriele Paoloni
>> Cc: Wangzhou (B); <[e-mail]>; <[e-mail]>;
>> <[e-mail]>; Yuanzhichang; Zhudacai; zhangjukuo;
>> qiuzhenfa; Liguozhu (Kenneth); [e-mail]
>> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
>> 
>> On 2015. 8. 21., at PM 5:58, Gabriele Paoloni
>> <[e-mail]> wrote:
>>> 
>>> Hi Jingoo
>>> 
>>>> -----Original Message-----
>>>> From: [e-mail] [mailto:linux-pci-
>>>> [e-mail]] On Behalf Of Jingoo Han
>>>> Sent: Friday, August 21, 2015 5:48 AM
>>>> To: Wangzhou (B)
>>>> Cc: Gabriele Paoloni; <[e-mail]>; <linux-
>>>> [e-mail]>; <[e-mail]>; Yuanzhichang;
>>>> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
>>>> [e-mail]
>>>> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
>>>> 
>>>>> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]>
>> wrote:
>>>>> 
>>>>>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
>>>>>> From: gabriele paoloni <[e-mail]>
>>>>>> 
>>>>>> 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 <[e-mail]>
>>>>>> ---
>>>>>> 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 [e-mail]
>>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni Aug. 21, 2015, 12:40 p.m. UTC | #8

> -----Original Message-----
> From: Jingoo Han [mailto:[e-mail]]
> Sent: Friday, August 21, 2015 12:08 PM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); <[e-mail]>; <[e-mail]>;
> <[e-mail]>; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth); [e-mail]
> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> 
> On 2015. 8. 21., at PM 7:29, Gabriele Paoloni
> <[e-mail]> wrote:
> >
> >> -----Original Message-----
> >> From: Jingoo Han [mailto:[e-mail]]
> >> Sent: Friday, August 21, 2015 10:25 AM
> >> To: Gabriele Paoloni
> >> Cc: Wangzhou (B); <[e-mail]>; <[e-mail]>;
> >> <[e-mail]>; Yuanzhichang; Zhudacai; zhangjukuo;
> >> qiuzhenfa; Liguozhu (Kenneth); [e-mail]
> >> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> >>
> >> On 2015. 8. 21., at PM 5:58, Gabriele Paoloni
> >> <[e-mail]> wrote:
> >>>
> >>> Hi Jingoo
> >>>
> >>>> -----Original Message-----
> >>>> From: [e-mail] [mailto:linux-pci-
> >>>> [e-mail]] On Behalf Of Jingoo Han
> >>>> Sent: Friday, August 21, 2015 5:48 AM
> >>>> To: Wangzhou (B)
> >>>> Cc: Gabriele Paoloni; <[e-mail]>; <linux-
> >>>> [e-mail]>; <[e-mail]>; Yuanzhichang;
> >>>> Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
> >>>> [e-mail]
> >>>> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write
> >>>>
> >>>>> On 2015. 8. 21., at AM 10:51, Zhou Wang <[e-mail]>
> >> wrote:
> >>>>>
> >>>>>> On 2015/8/20 21:58, Gabriele Paoloni wrote:
> >>>>>> From: gabriele paoloni <[e-mail]>
> >>>>>>
> >>>>>> 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 <[e-mail]>
> >>>>>> ---
> >>>>>> 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 [e-mail]
> >>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Sept. 3, 2015, 8:59 p.m. UTC | #9

On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <[e-mail]>
> 
> 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.

I think there are two problems in this area:

1) Most calls of dw_pcie_cfg_write() are of the form:

    dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...)
    dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...)

   This is needlessly repetitive because the caller has to mention
   "where" twice and do the masking itself.  We could simply pass
   "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the
   required masking internally.

2) Pratyush, this one's for you :)  spear13xx_pcie_establish_link()
   calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times like
   this:

    dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)
    dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)

   I think these are incorrect because dw_pcie_cfg_read() and
   dw_pcie_cfg_write() don't add anything to the "addr" argument
   ("pp->dbi_base" in this case) except the 0-3 byte select offset, so
   they use the correct alignment, but access the wrong registers.

As far as the original patch below, I don't think I have a strong
opinion either way.  You could consider doing something like:

  if (size == 4) {
    if (addr & 3) return PCIBIOS_BAD_REGISTER_NUMBER;
    writel(val, addr);
  } else if (size == 2) {
    if (addr & 1) return PCIBIOS_BAD_REGISTER_NUMBER;
    writew(val, addr);
  } ...

This is similar to what we do in pci_bus_write_config_word() (see
drivers/pci/access.c).

> Signed-off-by: Gabriele Paoloni <[e-mail]>
> ---
>  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
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pratyush Anand Sept. 4, 2015, 6:06 a.m. UTC | #10

Hi Bjorn,


On Fri, Sep 4, 2015 at 2:29 AM, Bjorn Helgaas <[e-mail]> wrote:
> On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote:
>> From: gabriele paoloni <[e-mail]>
>>
>> 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.
>
> I think there are two problems in this area:
>
> 1) Most calls of dw_pcie_cfg_write() are of the form:
>
>     dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...)
>     dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...)
>
>    This is needlessly repetitive because the caller has to mention
>    "where" twice and do the masking itself.  We could simply pass
>    "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the
>    required masking internally.

I think just by passing "pp->dbi_base" and "where"  we can not take
care of all cases. For example, if I have to read PCI_CACHE_LINE_SIZE
(8 bit value at offset 0x0C), I will have to pass "pp->dbi_base" +
PCI_CACHE_LINE_SIZE, 1

So, I think User of this function will need to pass "addr","size" and
"val", where
"addr"  as "pp->dbi_base" + "where"
"size" and "val" as it is.

one more thing IIRC, there was some discussion (however I am unable to
find any reference), and not sure if it is still true with any
platform:  issues with non word aligned PCI register read.
If a platform is not able to read non word aligned register correctly,
then in that case we will have to pass where and size both :(

>
> 2) Pratyush, this one's for you :)  spear13xx_pcie_establish_link()
>    calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times like
>    this:
>
>     dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)
>     dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)
>
>    I think these are incorrect because dw_pcie_cfg_read() and
>    dw_pcie_cfg_write() don't add anything to the "addr" argument
>    ("pp->dbi_base" in this case) except the 0-3 byte select offset, so
>    they use the correct alignment, but access the wrong registers.

hummm..so for sure the version which was up-streamed has never been
tested with buggy gen1 card and with any endpoint which would have
generated a read request of more than 128 bytes :( .. Will correct.

Thanks a lot for pointing this out :-)

~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni Sept. 8, 2015, 4:19 p.m. UTC | #11

Hi Bjorn and Pratyush, thanks for reviewing

> -----Original Message-----

> From: [e-mail] [mailto:linux-pci-

> [e-mail]] On Behalf Of Pratyush Anand

> Sent: Friday, September 04, 2015 7:06 AM

> To: Bjorn Helgaas

> Cc: Gabriele Paoloni; [e-mail]; Wangzhou (B); Jingoo

> Han; Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth)

> Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write

> 

> Hi Bjorn,

> 

> 

> On Fri, Sep 4, 2015 at 2:29 AM, Bjorn Helgaas <[e-mail]>

> wrote:

> > On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote:

> >> From: gabriele paoloni <[e-mail]>

> >>

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

> >

> > I think there are two problems in this area:

> >

> > 1) Most calls of dw_pcie_cfg_write() are of the form:

> >

> >     dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...)

> >     dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...)

> >

> >    This is needlessly repetitive because the caller has to mention

> >    "where" twice and do the masking itself.  We could simply pass

> >    "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the

> >    required masking internally.

> 

> I think just by passing "pp->dbi_base" and "where"  we can not take

> care of all cases. For example, if I have to read PCI_CACHE_LINE_SIZE

> (8 bit value at offset 0x0C), I will have to pass "pp->dbi_base" +

> PCI_CACHE_LINE_SIZE, 1


Say you call dw_pcie_cfg_write(pp->dbi_base, PCI_CACHE_LINE_SIZE, 1, ...)

you can have:

int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
{
	addr += where;
	if (size == 4) {
		if (addr & 3) return PCIBIOS_BAD_REGISTER_NUMBER;
		writel(val, addr);
	} else if (size == 2) {
		if (addr & 2) return PCIBIOS_BAD_REGISTER_NUMBER;
		writew(val, addr + (where & 2));
	} else if (size == 1)
		writeb(val, addr + (where & 3));
	else
		return PCIBIOS_BAD_REGISTER_NUMBER;

	return PCIBIOS_SUCCESSFUL;
}

So we can clean all the callers that Bjorn pointed out....

> 

> So, I think User of this function will need to pass "addr","size" and

> "val", where

> "addr"  as "pp->dbi_base" + "where"

> "size" and "val" as it is.

> 

> one more thing IIRC, there was some discussion (however I am unable to

> find any reference), and not sure if it is still true with any

> platform:  issues with non word aligned PCI register read.

> If a platform is not able to read non word aligned register correctly,

> then in that case we will have to pass where and size both :(

> 

> >

> > 2) Pratyush, this one's for you :)  spear13xx_pcie_establish_link()

> >    calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times

> like

> >    this:

> >

> >     dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL,

> 4, ...)

> >     dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL,

> 4, ...)

> >

> >    I think these are incorrect because dw_pcie_cfg_read() and

> >    dw_pcie_cfg_write() don't add anything to the "addr" argument

> >    ("pp->dbi_base" in this case) except the 0-3 byte select offset,

> so

> >    they use the correct alignment, but access the wrong registers.

> 

> hummm..so for sure the version which was up-streamed has never been

> tested with buggy gen1 card and with any endpoint which would have

> generated a read request of more than 128 bytes :( .. Will correct.


If you are happy with the solution above I can create a patchset, where
the first patch fixes this and the second changes dw_pcie_cfg_write(),
dw_pcie_cfg_read and respective function calls...

> 

> Thanks a lot for pointing this out :-)

> 

> ~Pratyush

> --

> To unsubscribe from this list: send the line "unsubscribe linux-pci" in

> the body of a message to [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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