On Sat, Feb 12, 2022 at 02:42:16PM -0700, Ted Bullock wrote:
> On 2022-02-11 7:16 p.m., Jonathan Gray wrote:
> > I'm not so keen on another function and putting bar information into
> > local arrays seems odd.
>
> Are there technical reasons for not wanting a function? I know that
> pulling in the linux kernel code is probably a big headache, does it
> have to do with that?
Look at other pci drivers and you will see they deal with
bars in attach.
>
> As to the arrays, I chose to use a idiom for searching the BARs
> deliberately that (to me) reads as dramatically simpler and clearer. The
> BAR code already in tree for this driver is a mess with inline global
> defines sometimes, direct hex codes other times and assigned variables
> yet other times.
Drivers normally look at one bar and have a define for the offset
used in attach. Iterating over bars and generation specific tests
aren't the norm. The linux interfaces all want to deal with a bar
number instead of of offset. For radeon linux does it in
radeon_device_init().
>
> > It would be easier to review changes if you would stick to either
> > changing or moving code, not both at the same time.
>
> Yeah, moving/changing does make the review harder sorry about that :(
>
> > For example when iterating over bars you lost the part that skips one
> > if a bar is 64-bit.
>
> No. This was deliberate and not lost, I looked at this very carefully.
>
> bar[i] && PCI_MAPREG_TYPE(type[i]) == PCI_MAPREG_TYPE_IO
>
> This should not match to the top half of a 64bit memory mapped
> BAR, so having a special code to modify the loop to handle such a
> condition is just adding needless complexity.
>
> For a memory BAR, bit 0 is always 0.
> For an IO BAR, bit 0 is 1.
>
> Since this loop is explicitly searching for an IO bar, we should not
> need to handle MM semantics when searching, the simple match for the
> first bit should be enough. Is this incorrect?
>
> > Generally bool is avoided in kernel code, it was added to not have
> > to change drm code.
>
> No problem, the function is probably more correctly written with integer
> returns accurately describing problems from errno.h
>
> > We use 'if (' not 'if(' as well.
>
> Oops; I missed this despite staring at the code for a week.
>
> Thanks for taking the time to review :) Much appreciated, tentatively I
> left the function in but with the changed return type and adjusted to
> your other notes, if it is a problem this can of course be amended.
I will review further when you drop the function.
