Message-ID: <CAFCwf13Lwr7M2Jao+3k+w_VaQ-9J7dpu7AOS4B+udnG__Fd53A@mail.gmail.com>
Date:   Thu, 14 Feb 2019 12:45:00 +0200
From:   Oded Gabbay <oded.gabbay@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Olof Johansson <olof@...om.net>, ogabbay@...ana.ai,
        Arnd Bergmann <arnd@...db.de>, Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v4 00/15] Habana Labs kernel driver

On Thu, Feb 14, 2019 at 12:37 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Thu, Feb 14, 2019 at 12:15:19PM +0200, Oded Gabbay wrote:
> > On Thu, Feb 14, 2019 at 12:07 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Thu, Feb 14, 2019 at 11:58:41AM +0200, Oded Gabbay wrote:
> > > > On Thu, Feb 14, 2019 at 9:13 AM Oded Gabbay <oded.gabbay@...il.com> wrote:
> > > > >
> > > > > On Thu, Feb 14, 2019 at 9:11 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > > >
> > > > > > On Mon, Feb 11, 2019 at 05:17:36PM +0200, Oded Gabbay wrote:
> > > > > > > Hello,
> > > > > > > This is v4 of the Habana Labs kernel driver patch-set. It contains fixes
> > > > > > > according to reviews done on v3, mainly for the command buffer, sysfs and MMU
> > > > > > > patches. In addition, patch 2/15 was reduced in size from 4.3MB to 1.4MB.
> > > > > > >
> > > > > > > The patch-set is rebased on v5.0-rc6.
> > > > > > >
> > > > > > > Link to v3 cover letter: https://lkml.org/lkml/2019/2/4/1033
> > > > > > >
> > > > > > > Link to v2 cover letter: https://lkml.org/lkml/2019/1/30/1003
> > > > > > >
> > > > > > > Link to v1 cover letter: https://lwn.net/Articles/777342/
> > > > > > >
> > > > > > > I would appricate any feedback, question and/or review.
> > > > > >
> > > > > > There's been some 0-day bot feedback on some of these patches now that I
> > > > > > put them in my -testing branch.  So I'm going to drop the patch series
> > > > > > from there now and wait for a v5 of the series that hopefully will have
> > > > > > those issues fixed :)
> > > > > >
> > > > Hi Greg,
> > > > I looked at the 4 warnings I received from your emails, and they all
> > > > appear in i386 architecture.
> > > > I don't want to support 32-bit kernel and I don't intend to support it.
> > > > Can we just specify in kconfig that we don't support it, and then you
> > > > won't get these warnings ?
> > >
> > > No, if you use the correct kernel types and castings, you should be
> > > fine.
> > >
> > > > I initially set in kconfig to support only x86_64, and you told me
> > > > (and you were right) not to limit to that. But I do think I would like
> > > > to disable the driver on i386.
> > >
> > > You might want to not support it on 32bit kernels, but even then, I
> > > think all you need to do here is use the proper kernel types and you
> > > will be ok.
> > >
> > > As an example:
> > >    drivers/misc/habanalabs/goya/goya.c: In function 'goya_early_init':
> > >    drivers/misc/habanalabs/goya/goya.c:404:4: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
> > >        "Not " HL_NAME "? BAR %d size %llu, expecting %llu\n",
> > >        ^~~~~~
> > >
> > > Use the correct printk type for a resource_size_t.
> > >
> > > You got that warning twice.
> > >
> > > Another one is:
> > > >> drivers/misc/habanalabs/device.c:283:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> > >      volatile u32 *paddr = (volatile u32 *) addr;
> > >
> > > Now using a volatile makes me want to say "you are doing it wrong!", as
> > > yes, you shouldn't be reading directly from a memory pointer, you need
> > > to use the correct iomem accessors, right?
> > >
> > > So I think just fixing this stuff up should be simple, the
> > > resource_size_t fix is needed no matter what size kernel you run on.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > ok, got it, will be fixed.
> >
> > Regarding the volatile, this is not an I/O memory. This is host memory
> > that is changed by the device. That's why I wrote in the comment
> > there:
> > /*
> > * paddr is defined as volatile because it points to HOST memory,
> > * which is being written to by the device. Therefore, we can't use
> > * locks to synchronize it and it is not a memory-mapped register space
>
> What do you mean by "HOST" memory?  The memory that the processor is
> running on?
>
Yes, exactly. The memory of the server. Not a memory on my device.

> > */
> >
> > Am I missing something here ? I don't think I should use the iomem
> > accessors on host memory, right ? Assuming I'm right, is there another
> > way to ensure the compiler won't optimize this without using the
> > volatile keyword ?
>
> What are you trying to prevent from being "optimized" here?
>
> Are you sure you just don't need a correct memory barrier?  That's the
> only way to ensure that if you write to a location from one thread/cpu,
> it will show up to the other thread/cpu correctly.  volatile will not
> ensure that for you at all (hint, the compiler just ignores it for the
> most part.)

But the writing entity in this case is NOT another thread/cpu. The
writing entity is the device. So a memory barrier, IMO, won't help me
here, because memory barriers affect only on the CPU. Not on external
initiators.

AFAIK, the volatile keyword tells the compiler that the value of the
variable may change at any time--without any action being taken by the
code the compiler finds nearby. And this is exactly what happens here.
I poll on a memory location of the CPU, and that memory can change at
any time by the device.

Thanks,
Oded


>
> thanks,
>
> greg k-h

Powered by blists - more mailing lists