On Fri, Nov 15, 2024 at 11:43:22AM -0500, Frank Li wrote:
> On Fri, Nov 15, 2024 at 10:53:07AM +0100, Niklas Cassel wrote:
> > On Thu, Nov 14, 2024 at 05:52:39PM -0500, Frank Li wrote:
> > > +static inline int pci_epf_align_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > > + u32 low, u32 high, u64 *base, size_t *off)
> > > +{
> > > + u64 addr = high;
> > > +
> > > + addr <<= 32;
> > > + addr |= low;
> > > +
> > > + return pci_epf_align_addr(epf, bar, addr, base, off);
> > > +}
> >
> > I'm not sure if this function deserves to live :)
> > Can't the caller just do this before calling pci_epf_align_addr() ?
>
> Ideally, kernel should have macro to combine 32bit macro to a 64bit, but
> I have not found it.
>
> It is quite easy to make error or warning by simple
> (high << 32 | low)
>
> It needs ((u64) high << 32 | low) at least. I just want to avoid everyone
> to struggle simple issue.
>
> And msi_msg use lo and hi. pci_function_test actually just demostrate how
> to use doorbell. if other function driver use doorbell in future, avoid
> "(u64) high << 32 | low" copy to everywhere.
Keeping the helper is fine with me, but it probably should be renamed
to include _ib_ or _inbound_ in the function name, similar to the comment
for pci_epf_align_addr().
Kind regards,
Niklas
