Re: [PATCH 01/15] habanalabs: add skeleton driver

From: Oded Gabbay <oded.gabbay@gmail.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: gregkh <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ogabbay@habana.ai
Subject: Re: [PATCH 01/15] habanalabs: add skeleton driver
Date: Sat, 26 Jan 2019 23:48:02 +0200	[thread overview]
Message-ID: <CAFCwf109SsyUVfvWAwA-j6HbqJ-QEZkWUhwirtX7dUf5CP_R7A@mail.gmail.com> (raw)
In-Reply-To: <CAK8P3a3=1Rocb8jcG8XHadgcUCL6xXs8GRxc-Az6oyPqffYTpw@mail.gmail.com>

On Sat, Jan 26, 2019 at 11:14 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Jan 26, 2019 at 5:25 PM Oded Gabbay <oded.gabbay@gmail.com> wrote:
> >
> > On Sat, Jan 26, 2019 at 6:06 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Wed, Jan 23, 2019 at 1:01 AM Oded Gabbay <oded.gabbay@gmail.com> wrote:
> > >
> > > > diff --git a/drivers/misc/habanalabs/include/habanalabs_device_if.h b/drivers/misc/habanalabs/include/habanalabs_device_if.h
> > > > new file mode 100644
> > > > index 000000000000..9dbb7077eabd
> > > > --- /dev/null
> > > > +++ b/drivers/misc/habanalabs/include/habanalabs_device_if.h
> > >
> > > Since this is a apparently a user space ABI, the file should be in
> > > include/uapi/linux/,
> > > not in the driver directory.
> >
> > This is not a user space ABI. This is the ABI between the driver and the F/W.
>
> Ah, I see. In that case, you should get rid of all the bitfields and make the
> struct members all __le32/__le64/... to make it work on big-endian kernels.
>
I really don't want to start converting bitfields and structures to
use __le32/64.
As I wrote in one of the previous reviews, we don't support big-endian
architecture (what's left after POWER moved to support little endian
?).  We actually do run on POWER9 but with ppc64le architecture
In any case, our software stack is so big that this minor change in
the driver won't have any impact on the overall ability to run
something on our H/W

> > >
> > > > +/* must be aligned to 4 bytes */
> > > > +struct armcp_info {
> > > > +       struct armcp_sensor sensors[ARMCP_MAX_SENSORS];
> > > > +       __u8 kernel_version[VERSION_MAX_LEN];
> > > > +       __u32 reserved[3];
> > > > +       __u32 cpld_version;
> > > > +       __u32 infineon_version;
> > > > +       __u8 fuse_version[VERSION_MAX_LEN];
> > > > +       __u8 thermal_version[VERSION_MAX_LEN];
> > > > +       __u8 armcp_version[VERSION_MAX_LEN];
> > > > +       __u64 dram_size;
> > > > +};
> > >
> > > The compiler will align this to 8 bytes on most architectures, and
> > > add another padding field before dram_size. Better remove the
> > > 'reserved' fields, or make them an even number.
> > I can't do that, because those fields were once used by the F/W and if
> > I will change the order here, or add/remove those fields then it will
> > break compatibility with old F/W.
>
> Ok, I see. Then you should add an explicit padding field and fix the
> comment to make the structure match the actual interface.
>
>        Arnd
Understood, will be fixed.
Thanks,
Oded