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 :)
Kind regards,
Niklas
