On Mon, Mar 04, 2024 at 09:04:30PM +0000, Niklas Cassel wrote:
> On Mon, Mar 04, 2024 at 02:13:27PM -0500, Frank Li wrote:
> > >
> > > Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> > > placeholder sounds good to me. Please use that instead.
> > >
> >
> > It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
> > Niklas's initial suggestion have not consider this condition. If u8 have to
> > change to u16 or s16 anyways, I prefer use -1 as free.
>
> Well, to be fair, my suggestion was:
> "If we continue to use a u8, and offset the saved value by one,
> we will at least be able to support 255-1 == 254 iATUs."
>
> But we have this define:
> drivers/pci/controller/dwc/pcie-designware.h:#define MAX_IATU_IN 256
> (Even if it isn't used anywhere.)
>
> But as ridiculous as it may seem to support that many inbound ranges,
> that is the max number of windows supported by the hardware, so why
> not just let the driver support the max supported by the hardware?
>
>
> We are talking about:
> struct dw_pcie_ep {
> ...
> u8 bar_to_atu[PCI_STD_NUM_BARS];
> ...
> }
>
> where PCI_STD_NUM_BARS == 6.
>
> And where struct dw_pcie_ep is kzalloced for what I assume is all drivers.
>
> So I'm actually for your idea of changing it to u16, or even unsigned int.
>
> If the code is simplified if we use a u16 or unsigned int (because we don't
> need any weird if (val < MAX_IATU_IN - 1) check), then I'm all for it.
>
>
> What I personally don't like with your patch in $subject,
> was that you changed both dw_pcie_ep_clear_bar() to set the "clear value"
> to -1, but you also need a
> memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu)); in dw_pcie_ep_init().
>
>
> I much prefer to have 0 as the default/unused value, because it will
> automatically get set when you do kzalloc().
> Seeing a memset -1 just looks a bit hackish to be, but I realize that
> it is personal preference.
>
>
> If it is super important to save 8 bytes from the heap, then I would
> even prefer changing the MAX_IATU_IN 256 to something smaller, like
> 127 or whatever, just as long as we don't need that extra memset -1 :)
Okay, Let me work on '0' version.
Frank
>
>
> Kind regards,
> Niklas
