RE: [PATCH] NTB: Register and offset values fix for memory window

["Allen Hubbe" <Allen.Hubbe@dell.com>]

From: Shyam Sundar S K
> On 11/17/2016 9:09 PM, Jon Mason wrote:
> > On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
> >> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
> >>> From: Shyam Sundar S K
> >>>> Due to incorrect limit and translation register values, NTB link was
> >>>> going down when the memory window translation was setup. Made appropriate
> >>>> changes as per spec.
> >>>>
> >>>> Also, fixed the limit register values for BAR1, which was overlapping
> >>>> with the BAR23 address.
> >>>>
> >>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> >>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> >>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
> >>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> >>>> ---
> >>>
> >>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
> >>>>  {
> >>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
> >>>>      void __iomem *mmio = ndev->self_mmio;
> >>>> -    u32 offset;
> >>>>
> >>>> -    if (idx < 0 || idx >= ndev->spad_count)
> >>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
> >>>
> >>> Why does this change add four to the upper end of the range check?  Does spad_count
> have the wrong number of spads?
> >>>
> >>>>              return 0;
> >>>>
> >>>> -    offset = ndev->self_spad + (idx << 2);
> >>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
> >>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
> >>>
> >>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the
> first or second half of the spads, and the peer self_spad is the other half.  From this
> change, can we assume that a single-ntb topology will not be supported?
> >>>
> >> This patch has more to with the NTB driver becoming functional on AMD platforms.
> >> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW
> spec. So changed it accordingly.
> >
> > I believe the point Alan was making was that if the SPAD count is +4,
> > then perhaps the value in spad_count should be +4.  Are there 16 or 20
> > SPADs?
> >
> 
> There are 16 SPAD in our case, if I don't add that hack here eth link does not become
> ready.
> 
> meaning, in ntb_transport: this never gets set "nt->link_is_up = true;" reason being
> ntb_spad_read(ndev, MW0_SZ_HIGH + (i * 2)); and ntb_spad_read(ndev, MW0_SZ_LOW + (i * 2));
> so, calls to ntb_spad_read() will have idx more than the ndev->spad_count.
> 

In MW0_SZ_LOW + (i * 2), 'i' is in the range of the number of memory windows, which is greater on AMD.  The ntb_transport driver as currently written will require more scratchpads for initialization on AMD, because the AMD driver advertises more memory windows.

The changes here in ntb_hw_amd are incorrect, because they do allow read and write access out of bounds of the hardware scratchpad registers.  This only appears to work, because ntb_netdev only creates one queue in ntb_transport.  Because ntb_netdev only creates one queue, we don't observe any trouble with the misconfigured third memory window, which is configured with bogus values from non-scratchpad hardware registers.  If a driver allocated a third queue, I expect there would be trouble.

> Any suggestions ?
>

Instead, limit the number of memory windows actually used by ntb_transport, such that ntb_transport can configure those memory windows using only the available scratchpads.  If there are not enough scratchpads to exchange values for all of the memory windows, then ntb_transport should limit itself to use fewer memory windows.