Re: [Outreachy kernel] Re: [PATCH v2 1/5] staging: octeon: remove typedef declaration for cvmx_wqe

[Paul Burton]

Hi Wambui, Julia,

Side-note: Wambui, your mail client seems to have added this header:

  Reply-To: alpine.DEB.2.21.1910240722070.2771@hadrien

This is the ID of the message you replied to (ie. this is the same value
that the In-Reply-To: header has). You should probably figure out how
that happened, or you're going to miss responses when people reply
without noticing the bogus email address.

On Thu, Oct 24, 2019 at 01:00:20PM +0300, Wambui Karuga wrote:
> On Thu, Oct 24, 2019 at 07:26:59AM +0200, Julia Lawall wrote:
> > > If you're making significant changes to this driver, please test them
> > > using the MIPS cavium_octeon_defconfig which is where this driver is
> > > actually used.
> > >
> > > This driver has broken builds a few times recently which makes me very
> > > tempted to ask that we stop allowing it to be built with COMPILE_TEST.
> > > The whole octeon-stubs.h thing is a horrible hack anyway...
> > 
> > Wambui,
> > 
> > Please figure out what went wrong here.  Are there two sets of typedefs
> > that should have been updated?
> >
> I managed to reproduce these build errors and finally noticed that the
> "octeon-stubs.h" header is only included when CONFIG_CAVIUM_OCTEON_SOC
> is not defined, therefore compiling properly for COMPILE_TEST but will
> actually fail when compiled with CONFIG_CAVIUM_OCTEON_SOC is set since
> the functions will try to use the definitions in
> arch/mips/include/asm/octeon/ that don't have the changes.
> 
> Paul, please tell me if this is correct?

Yes, that's correct. The driver was written to use the headers in
arch/mips/include/asm/octeon, and then recently the octeon-stubs.h
header was added which duplicates lots of the MIPS & Octeon-specific
header content in one huge monstrous file.

I'm all for improving compile test coverage, but I think octeon-stubs.h
in its short life has already proven itself to be a bad way to achieve
that goal[1][2][3].

> > Others,
> > 
> > Would it be reasonable to put the information about how the driver should
> > be compied in the TODO file?  git grep cavium_octeon_defconfig in the
> > octeon directory turns up nothing.

It wouldn't hurt. I'd argue that Kconfig already provides enough
information to figure this out easily though - OCTEON_ETHERNET depends
on CAVIUM_OCTEON_SOC || COMPILE_TEST, which ought to tell people that
its real use is when CAVIUM_OCTEON_SOC=y. That doesn't necessarily point
you to cavium_octeon_defconfig (though grepping for CAVIUM_OCTEON_SOC=y
would), but that's not strictly needed anyway - any old config with
CAVIUM_OCTEON_SOC=y would do.

Thanks,
    Paul

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0228ecf6128c92b47eadd2ac270c5574d9150c09
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/staging/octeon?id=17a29fea086ba18b000d28439bd5cb4f2b0a527b
[3] https://storage.kernelci.org/next/master/next-20191024/mips/cavium_octeon_defconfig/gcc-8/build.log