Re: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

Re: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

[Pratyush Anand]

Hi Tejun,

Its a long pending patch.
Any decision on this patch.

Regards
Pratyush

On 3/16/2011 12:35 PM, pratyush wrote:
> Hello Hemant,
>
> On 3/16/2011 9:43 AM, Pedanekar, Hemant wrote:
>>> What happens in your case? Does it not work, if you do not
>>>> modify sil driver?
>>>>
>> I also require to modify the sil driver to avoid setting max RRQ to 4096
>> otherwise the data transfer over PCIe does not work. The RC responds with
>> "Unsupported Request" for read request of size greater than 256 bytes.
>>
>> The other problem is, there is no way for peers to know that the other
>> device cannot handle RRQ of size beyond certain limit. At least I am not
>> aware of any bits in PCIe configuration space which advertise this. Do you
>> know of any such?
>>
>> While, there is "Max RRQ size" field in device control register which only
>> controls outbound RRQ size.
>>
>> This restriction probbaly will complicate more when the device with such
>> RRQ limitation is a EP, in such case, no one else than the EP (and its driver)
>> would know about this limitation and need to force others (including RC) to
>> reduce the max RRQ size.
>>
>
> Yes, I am agreed with you. I do not see any way to inform its peers about request
> limit. So I think that a EP driver should not be allowed to call any function which
> changes EP's RRQ size. It should always be RC driver which sets these values as
> per max capability of the link.
>
> Currently I am managing it with postinit hook of RC driver. But can be part of
> common pci stack.
>
> You can see my way of handling it at
> http://www.spinics.net/lists/linux-pci/msg09897.html
>
> Regards
> Pratyush

Re: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

[Tejun Heo]

Hello,

On Thu, Feb 23, 2012 at 03:31:29PM +0530, Pratyush Anand wrote:
> Its a long pending patch.
> Any decision on this patch.

Hmmm.... I really don't know what to do.  IIRC it was something lifted
from the proprietary sil driver and they probably added that to work
around performance oddities under certain configurations and I'm a bit
worried about changing it after all this time.  Is there any way to
make this conditional somehow?

Thanks.

-- 
tejun

Re: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

[Pratyush Anand]

On 2/23/2012 11:12 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Feb 23, 2012 at 03:31:29PM +0530, Pratyush Anand wrote:
>> Its a long pending patch.
>> Any decision on this patch.
>
> Hmmm.... I really don't know what to do.  IIRC it was something lifted
> from the proprietary sil driver and they probably added that to work
> around performance oddities under certain configurations and I'm a bit
> worried about changing it after all this time.  Is there any way to
> make this conditional somehow?

I see that there is a patch in the latest kernel.

commit a1c473aa11e61bc871be16279c9bf976acf22504
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Fri Oct 14 14:56:15 2011 -0500

     pci: Clamp pcie_set_readrq() when using "performance" settings

I think, this will resolve the issue.
We are moving to the new kernel. Will check it there.

Regards
Pratyush

>
> Thanks.
>

RE: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

[Pedanekar, Hemant]

Hi,

Pratyush Anand wrote on Friday, February 24, 2012 9:28 AM:

> On 2/23/2012 11:12 PM, Tejun Heo wrote:
>> Hello,
>> 
>> On Thu, Feb 23, 2012 at 03:31:29PM +0530, Pratyush Anand wrote:
>>> Its a long pending patch.
>>> Any decision on this patch.
>> 
>> Hmmm.... I really don't know what to do.  IIRC it was something lifted
>> from the proprietary sil driver and they probably added that to work
>> around performance oddities under certain configurations and I'm a bit
>> worried about changing it after all this time.  Is there any way to
>> make this conditional somehow?
> 
> I see that there is a patch in the latest kernel.
> 
> commit a1c473aa11e61bc871be16279c9bf976acf22504
> Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date:   Fri Oct 14 14:56:15 2011 -0500
> 
>      pci: Clamp pcie_set_readrq() when using "performance" settings
> 
> I think, this will resolve the issue.
> We are moving to the new kernel. Will check it there.
> 
I see that the patch above considers MPS to clamp RRQ size, while I think
both should be independent, or perhaps I am missing something. 

Not allowing EP drivers to choose max RRQ setting for their respective 
devices will prevent breaking the mismatches where EP sends RRQ which RC 
cannot handle.

Other option is that the EP drivers which force max RRQ size also handle
the case where they get completor abort (or it wil be UR?) from the 
completor and then try to use lower (or lowest) read request. This will 
prevent breaking the data paths at the same time allowing systems which are
capable to have max performance.

Thanks.

   Hemant

Re: [PATCH] PCIE: pcie_set_readrq must not be allowed by PCIE EP driver

[Pratyush Anand]

On 2/24/2012 11:08 AM, Pedanekar, Hemant wrote:
> Hi,
>
> Pratyush Anand wrote on Friday, February 24, 2012 9:28 AM:
>
>> On 2/23/2012 11:12 PM, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Thu, Feb 23, 2012 at 03:31:29PM +0530, Pratyush Anand wrote:
>>>> Its a long pending patch.
>>>> Any decision on this patch.
>>>
>>> Hmmm.... I really don't know what to do.  IIRC it was something lifted
>>> from the proprietary sil driver and they probably added that to work
>>> around performance oddities under certain configurations and I'm a bit
>>> worried about changing it after all this time.  Is there any way to
>>> make this conditional somehow?
>>
>> I see that there is a patch in the latest kernel.
>>
>> commit a1c473aa11e61bc871be16279c9bf976acf22504
>> Author: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>> Date:   Fri Oct 14 14:56:15 2011 -0500
>>
>>       pci: Clamp pcie_set_readrq() when using "performance" settings
>>
>> I think, this will resolve the issue.
>> We are moving to the new kernel. Will check it there.
>>
> I see that the patch above considers MPS to clamp RRQ size, while I think
> both should be independent, or perhaps I am missing something.
>

Please also see following patch for completeness.

commit b03e7495a862b028294f59fc87286d6d78ee7fa1
Author: Jon Mason <mason@myri.com>
Date:   Wed Jul 20 15:20:54 2011 -0500

     PCI: Set PCI-E Max Payload Size on fabric

Regards
Pratyush