On Thu, Feb 14, 2019 at 12:45:00PM +0200, Oded Gabbay wrote: > On Thu, Feb 14, 2019 at 12:37 PM Greg KH <gre...@linuxfoundation.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 <gre...@linuxfoundation.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.gab...@gmail.com> > > > > > wrote: > > > > > > > > > > > > On Thu, Feb 14, 2019 at 9:11 AM Greg KH > > > > > > <gre...@linuxfoundation.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.
And how is that memory location mapped into the device memory? As such, it's iomemory, right? volatile doesn't tell the compiler much, if anything, anymore. I can't seem to trace back the code here (it's split across multiple emails), but it seems that the memory location is coming from struct armcp_packet in one location, and a dma pool in another one. I don't know what the rules are for dma mapped memory, but it feels like 'volatile' is not the way to use it :) thanks, greg k-h