mbox series

[v2,0/7] firmware: add partial read support in request_firmware_into_buf

Message ID 20200220004825.23372-1-scott.branden@broadcom.com (mailing list archive)
Headers show
Series firmware: add partial read support in request_firmware_into_buf | expand

Message

Scott Branden Feb. 20, 2020, 12:48 a.m. UTC
This patch series adds partial read support in request_firmware_into_buf.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.

Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.

Scott Branden (7):
  fs: introduce kernel_pread_file* support
  firmware: add offset to request_firmware_into_buf
  test_firmware: add partial read support for request_firmware_into_buf
  firmware: test partial file reads of request_firmware_into_buf
  bcm-vk: add bcm_vk UAPI
  misc: bcm-vk: add Broadcom VK driver
  MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver

 MAINTAINERS                                   |    7 +
 drivers/base/firmware_loader/firmware.h       |    5 +
 drivers/base/firmware_loader/main.c           |   49 +-
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/bcm-vk/Kconfig                   |   42 +
 drivers/misc/bcm-vk/Makefile                  |   11 +
 drivers/misc/bcm-vk/bcm_vk.h                  |  357 +++++
 drivers/misc/bcm-vk/bcm_vk_dev.c              | 1197 +++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.c              | 1359 +++++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.h              |  210 +++
 drivers/misc/bcm-vk/bcm_vk_sg.c               |  273 ++++
 drivers/misc/bcm-vk/bcm_vk_sg.h               |   60 +
 drivers/misc/bcm-vk/bcm_vk_tty.c              |  327 ++++
 drivers/soc/qcom/mdt_loader.c                 |    7 +-
 fs/exec.c                                     |   77 +-
 include/linux/firmware.h                      |    8 +-
 include/linux/fs.h                            |   15 +
 include/uapi/linux/misc/bcm_vk.h              |  117 ++
 lib/test_firmware.c                           |  139 +-
 .../selftests/firmware/fw_filesystem.sh       |   80 +
 21 files changed, 4305 insertions(+), 37 deletions(-)
 create mode 100644 drivers/misc/bcm-vk/Kconfig
 create mode 100644 drivers/misc/bcm-vk/Makefile
 create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c
 create mode 100644 include/uapi/linux/misc/bcm_vk.h

Comments

Randy Dunlap Feb. 20, 2020, 1:04 a.m. UTC | #1
Hi,

On 2/19/20 4:48 PM, Scott Branden wrote:
> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> new file mode 100644
> index 000000000000..c75dfb89a38d
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/Kconfig
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Broadcom VK device
> +#
> +config BCM_VK
> +	tristate "Support for Broadcom VK Accelerators"
> +	depends on PCI_MSI
> +	default m

Need to justify default m. Normally we don't add drivers as enabled unless
they are required for basic (boot) operation.

> +	help
> +	  Select this option to enable support for Broadcom
> +	  VK Accelerators.  VK is used for performing
> +	  specific video offload processing.  This driver enables
> +	  userspace programs to access these accelerators via /dev/bcm-vk.N
> +	  devices.
> +
> +	  If unsure, say N.
> +
> +if BCM_VK
> +
> +config BCM_VK_H2VK_VERIFY_AND_RETRY
> +	bool "Host To VK Verifiy Data and Retry"

	                 Verify

> +	help
> +	  Turn on to verify the data passed down to VK is good,
> +	  and if not, do a retry until it succeeds.

No timeout on that retry?

> +	  This is a debug/workaround on FPGA PCIe timing issues
> +	  but may be found useful for debugging other PCIe hardware issues.
> +	  Small performance loss by enabling this debug config.
> +	  For properly operating PCIe hardware no need to enable this.
> +
> +	  If unsure, say N.
> +
> +config BCM_VK_QSTATS
> +	bool "VK Queue Statistics"
> +	help
> +	  Turn on to enable Queue Statistics.
> +	  These are useful for debugging purposes.
> +	  Some performance loss by enabling this debug config.
> +	  For properly operating PCIe hardware no need to enable this.
> +
> +	  If unsure, say N.
> +
> +endif

cheers.
Greg KH Feb. 20, 2020, 7:47 a.m. UTC | #2
On Wed, Feb 19, 2020 at 04:48:24PM -0800, Scott Branden wrote:
> Add Broadcom VK driver offload engine.
> This driver interfaces to the VK PCIe offload engine to perform
> should offload functions as video transcoding on multiple streams
> in parallel.  VK device is booted from files loaded using
> request_firmware_into_buf mechanism.  After booted card status is updated
> and messages can then be sent to the card.
> Such messages contain scatter gather list of addresses
> to pull data from the host to perform operations on.

Why is this a tty driver?

Have you worked with the V4L developers to tie this into the proper
in-kernel apis for this type of functionality?

Using a tty driver seems like the totally incorrect way to do this, what
am I missing?

Also, do not make up random error values, you return "-1" a lot here,
that is not ok.  Please fix up to return the correct -Ewhatever values
instead.

thanks,

greg k-h
Scott Branden Feb. 21, 2020, 12:06 a.m. UTC | #3
Hi Randy,

On 2020-02-19 5:04 p.m., Randy Dunlap wrote:
> Hi,
>
> On 2/19/20 4:48 PM, Scott Branden wrote:
>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>> new file mode 100644
>> index 000000000000..c75dfb89a38d
>> --- /dev/null
>> +++ b/drivers/misc/bcm-vk/Kconfig
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Broadcom VK device
>> +#
>> +config BCM_VK
>> +	tristate "Support for Broadcom VK Accelerators"
>> +	depends on PCI_MSI
>> +	default m
> Need to justify default m. Normally we don't add drivers as enabled unless
> they are required for basic (boot) operation.
Will remove default m as not needed to boot.  Interesting other offload 
engines misc/ocxl/Kconfig and misc/cxl/Kconfig have default m.
>
>> +	help
>> +	  Select this option to enable support for Broadcom
>> +	  VK Accelerators.  VK is used for performing
>> +	  specific video offload processing.  This driver enables
>> +	  userspace programs to access these accelerators via /dev/bcm-vk.N
>> +	  devices.
>> +
>> +	  If unsure, say N.
>> +
>> +if BCM_VK
>> +
>> +config BCM_VK_H2VK_VERIFY_AND_RETRY
>> +	bool "Host To VK Verifiy Data and Retry"
> 	                 Verify
>
>> +	help
>> +	  Turn on to verify the data passed down to VK is good,
>> +	  and if not, do a retry until it succeeds.
> No timeout on that retry?
This is only enabled for debug purposes or fpga workarounds - no need 
for a timeout.
>
>> +	  This is a debug/workaround on FPGA PCIe timing issues
>> +	  but may be found useful for debugging other PCIe hardware issues.
>> +	  Small performance loss by enabling this debug config.
>> +	  For properly operating PCIe hardware no need to enable this.
>> +
>> +	  If unsure, say N.
>> +
>> +config BCM_VK_QSTATS
>> +	bool "VK Queue Statistics"
>> +	help
>> +	  Turn on to enable Queue Statistics.
>> +	  These are useful for debugging purposes.
>> +	  Some performance loss by enabling this debug config.
>> +	  For properly operating PCIe hardware no need to enable this.
>> +
>> +	  If unsure, say N.
>> +
>> +endif
> cheers.
Randy Dunlap Feb. 21, 2020, 12:21 a.m. UTC | #4
On 2/20/20 4:06 PM, Scott Branden wrote:
> Hi Randy,
> 
> On 2020-02-19 5:04 p.m., Randy Dunlap wrote:
>> Hi,
>>
>> On 2/19/20 4:48 PM, Scott Branden wrote:
>>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>>> new file mode 100644
>>> index 000000000000..c75dfb89a38d
>>> --- /dev/null
>>> +++ b/drivers/misc/bcm-vk/Kconfig
>>> @@ -0,0 +1,42 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +#
>>> +# Broadcom VK device
>>> +#
>>> +config BCM_VK
>>> +    tristate "Support for Broadcom VK Accelerators"
>>> +    depends on PCI_MSI
>>> +    default m
>> Need to justify default m. Normally we don't add drivers as enabled unless
>> they are required for basic (boot) operation.
> Will remove default m as not needed to boot.  Interesting other offload engines misc/ocxl/Kconfig and misc/cxl/Kconfig have default m.

Thanks.

Uh, yes, that is interesting.  They both depend on PPC_POWERNV,
which I know nothing about.

>>> +    help
>>> +      Select this option to enable support for Broadcom
>>> +      VK Accelerators.  VK is used for performing
>>> +      specific video offload processing.  This driver enables
>>> +      userspace programs to access these accelerators via /dev/bcm-vk.N
>>> +      devices.
>>> +
>>> +      If unsure, say N.
Scott Branden Feb. 21, 2020, 6:19 p.m. UTC | #5
On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
> On Wed, Feb 19, 2020 at 04:48:24PM -0800, Scott Branden wrote:
>> Add Broadcom VK driver offload engine.
>> This driver interfaces to the VK PCIe offload engine to perform
>> should offload functions as video transcoding on multiple streams
>> in parallel.  VK device is booted from files loaded using
>> request_firmware_into_buf mechanism.  After booted card status is updated
>> and messages can then be sent to the card.
>> Such messages contain scatter gather list of addresses
>> to pull data from the host to perform operations on.
> Why is this a tty driver?
We have a tty driver here as there are (multiple) console interfaces to 
the card.
The only viable interface to the card is through the PCIe bus.  We can't 
hook up cables to the card to get at the consoles.
As such we implemented a tty driver to access the console via a circular 
buffer in PCIe BAR space.

It is extremely useful.  You get console access to virtual serial ports 
connected to each processor inside the VK SoC.
Future enhancement is to connect the tty driver to use an MSIX interrupt 
rather than polling.
Once that is done lrz/sz transfers will be much quicker.  I'd also look 
at if I could use ser2net to get network access
for the processors on the VK SoC over this interface as well.
>
> Have you worked with the V4L developers to tie this into the proper
> in-kernel apis for this type of functionality?
We looked at the V4L model doesn't have any support for anything we are 
doing in this driver.
We also want a driver that doesn't care about video.  It could be 
offloading crypto or other operations.
We talked with Olof about all of this previously and he said leave it as 
a misc driver for now.
He was going to discuss at linux plumbers conference that we need some 
sort of offload engine model that such devices could fit into.
> Using a tty driver seems like the totally incorrect way to do this, what
> am I missing?
tty driver is used to provide console access to the processors running 
on vk.
Data is sent using the bcm_vk_msg interface by read/write operations 
from user space.
VK then gets the messages and DMA's the data to/from host memory when 
needed to process.
>
> Also, do not make up random error values, you return "-1" a lot here,
> that is not ok.  Please fix up to return the correct -Ewhatever values
> instead.
OK.
>
> thanks,
>
> greg k-h
Arnd Bergmann Feb. 22, 2020, 8:02 a.m. UTC | #6
On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
<scott.branden@broadcom.com> wrote:
> On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:

> > Have you worked with the V4L developers to tie this into the proper
> > in-kernel apis for this type of functionality?
> We looked at the V4L model doesn't have any support for anything we are
> doing in this driver.
> We also want a driver that doesn't care about video.  It could be
> offloading crypto or other operations.
> We talked with Olof about all of this previously and he said leave it as
> a misc driver for now.
> He was going to discuss at linux plumbers conference that we need some
> sort of offload engine model that such devices could fit into.

I see. Have you looked at the "uacce" driver submission? It seems
theirs is similar enough that there might be some way to share interfaces.

> > Using a tty driver seems like the totally incorrect way to do this, what
> > am I missing?
> tty driver is used to provide console access to the processors running
> on vk.
> Data is sent using the bcm_vk_msg interface by read/write operations
> from user space.
> VK then gets the messages and DMA's the data to/from host memory when
> needed to process.

In turn here, it sounds like you'd want to look at what drivers/misc/mic/
and the mellanox bluefield drivers are doing. As I understand, they have the
same requirements for console, but have a nicer approach of providing
abstract 'virtio' channels between the PCIe endpoint and the host, and
then run regular virtio based drivers (console, tty, block, filesystem,
network, ...) along with application specific ones to provide the custom
high-level protocols. This is also similar to what the drivers/pci/endpoint
(from the other end) as the drivers/ntb (pci host on both ends) frameworks
and of course the rpmsg/remoteproc framework do.

In the long run, I would want much more consolidation between the
low-level parts of all these frameworks, but moving your high-level
protocols to the same virtio method would sound like a step in the
direction towards a generialized framework and easier sharing of
the abstractions.

       Arnd
Greg KH Feb. 23, 2020, 10 a.m. UTC | #7
On Sat, Feb 22, 2020 at 09:02:44AM +0100, Arnd Bergmann wrote:
> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
> <scott.branden@broadcom.com> wrote:
> > On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
> 
> > > Have you worked with the V4L developers to tie this into the proper
> > > in-kernel apis for this type of functionality?
> > We looked at the V4L model doesn't have any support for anything we are
> > doing in this driver.
> > We also want a driver that doesn't care about video.  It could be
> > offloading crypto or other operations.
> > We talked with Olof about all of this previously and he said leave it as
> > a misc driver for now.
> > He was going to discuss at linux plumbers conference that we need some
> > sort of offload engine model that such devices could fit into.
> 
> I see. Have you looked at the "uacce" driver submission? It seems
> theirs is similar enough that there might be some way to share interfaces.
> 
> > > Using a tty driver seems like the totally incorrect way to do this, what
> > > am I missing?
> > tty driver is used to provide console access to the processors running
> > on vk.
> > Data is sent using the bcm_vk_msg interface by read/write operations
> > from user space.
> > VK then gets the messages and DMA's the data to/from host memory when
> > needed to process.
> 
> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
> and the mellanox bluefield drivers are doing. As I understand, they have the
> same requirements for console, but have a nicer approach of providing
> abstract 'virtio' channels between the PCIe endpoint and the host, and
> then run regular virtio based drivers (console, tty, block, filesystem,
> network, ...) along with application specific ones to provide the custom
> high-level protocols. This is also similar to what the drivers/pci/endpoint
> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
> and of course the rpmsg/remoteproc framework do.
> 
> In the long run, I would want much more consolidation between the
> low-level parts of all these frameworks, but moving your high-level
> protocols to the same virtio method would sound like a step in the
> direction towards a generialized framework and easier sharing of
> the abstractions.

I agree, please do not override the generic tty api with something so
hardware-specific like this as it really is not a serial device here.

thanks,

greg k-h
Olof Johansson Feb. 23, 2020, 11:55 p.m. UTC | #8
On Sat, Feb 22, 2020 at 12:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
> <scott.branden@broadcom.com> wrote:
> > On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
>
> > > Have you worked with the V4L developers to tie this into the proper
> > > in-kernel apis for this type of functionality?
> > We looked at the V4L model doesn't have any support for anything we are
> > doing in this driver.
> > We also want a driver that doesn't care about video.  It could be
> > offloading crypto or other operations.
> > We talked with Olof about all of this previously and he said leave it as
> > a misc driver for now.
> > He was going to discuss at linux plumbers conference that we need some
> > sort of offload engine model that such devices could fit into.
>
> I see. Have you looked at the "uacce" driver submission? It seems
> theirs is similar enough that there might be some way to share interfaces.

Uacce isn't a driver (or wasn't last time I looked at it, when it had
a different name). It's more of a framework for standardized direct HW
access from userspace, and relies on I/O virtualization to keep DMA
secure/partitioned, etc. VK is more of a classic PCIe device, it'll
handle DMA to/from the host, etc.

> > > Using a tty driver seems like the totally incorrect way to do this, what
> > > am I missing?
> > tty driver is used to provide console access to the processors running
> > on vk.
> > Data is sent using the bcm_vk_msg interface by read/write operations
> > from user space.
> > VK then gets the messages and DMA's the data to/from host memory when
> > needed to process.
>
> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
> and the mellanox bluefield drivers are doing. As I understand, they have the
> same requirements for console, but have a nicer approach of providing
> abstract 'virtio' channels between the PCIe endpoint and the host, and
> then run regular virtio based drivers (console, tty, block, filesystem,
> network, ...) along with application specific ones to provide the custom
> high-level protocols.

This has more value on the device than on the host, as far as I've
seen it used (if you want to boot Linux on it and have things
exposed).

virtio isn't necessarily a match if all you really want is a character
stream for a console and don't need (or have performance requirements
beyond what virtio offers) other types of communication.

> This is also similar to what the drivers/pci/endpoint
> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
> and of course the rpmsg/remoteproc framework do.

remoteproc is more about booting a tightly integrated device on an
embedded system. Also not a match here IMHO.

> In the long run, I would want much more consolidation between the
> low-level parts of all these frameworks, but moving your high-level
> protocols to the same virtio method would sound like a step in the
> direction towards a generialized framework and easier sharing of
> the abstractions.

For a simple naive console/character stream, doing something on top of
hvc might be easier -- it already does polling for you, etc.

Of course, the intent is not to ever use it as a console for the host
here, so that aspect of hvc isn't useful. But it gives you a bunch of
other stuff for free with just getchar/putchar interfaces to
implement.


-Olof
Scott Branden Feb. 25, 2020, 10:37 p.m. UTC | #9
Hi Olof/All,

I'm trying to digest all the feedback of what needs to be done.
I will be fixing up all the valuable comments about general issues but 
would like to know
what needs to be done about the tty interface.

The VK devices are configured to write serial data to circular buffers 
in memory or out a UART.
When we configure a system using the UART we connect a cable to the host 
and open a tty device.
When we don't connect a UART cable to the host we open the tty device in 
our driver instead.
In this case the memory is exposed to the host via PCI BAR memory space.
The bcm-vk host driver then accesses PCI space and presents a tty 
interface to the host.
We implemented a tty device to present the tty interface.
Host doesn't change anything other than opening a different devnode in 
UART vs. PCI case.

Based on all the comments: what interface should we be providing in 
driver instead?

On 2020-02-23 3:55 p.m., Olof Johansson wrote:
> On Sat, Feb 22, 2020 at 12:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
>> <scott.branden@broadcom.com> wrote:
>>> On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
>>>> Have you worked with the V4L developers to tie this into the proper
>>>> in-kernel apis for this type of functionality?
>>> We looked at the V4L model doesn't have any support for anything we are
>>> doing in this driver.
>>> We also want a driver that doesn't care about video.  It could be
>>> offloading crypto or other operations.
>>> We talked with Olof about all of this previously and he said leave it as
>>> a misc driver for now.
>>> He was going to discuss at linux plumbers conference that we need some
>>> sort of offload engine model that such devices could fit into.
>> I see. Have you looked at the "uacce" driver submission? It seems
>> theirs is similar enough that there might be some way to share interfaces.
> Uacce isn't a driver (or wasn't last time I looked at it, when it had
> a different name). It's more of a framework for standardized direct HW
> access from userspace, and relies on I/O virtualization to keep DMA
> secure/partitioned, etc. VK is more of a classic PCIe device, it'll
> handle DMA to/from the host, etc.
>
>>>> Using a tty driver seems like the totally incorrect way to do this, what
>>>> am I missing?
>>> tty driver is used to provide console access to the processors running
>>> on vk.
>>> Data is sent using the bcm_vk_msg interface by read/write operations
>>> from user space.
>>> VK then gets the messages and DMA's the data to/from host memory when
>>> needed to process.
>> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
>> and the mellanox bluefield drivers are doing. As I understand, they have the
>> same requirements for console, but have a nicer approach of providing
>> abstract 'virtio' channels between the PCIe endpoint and the host, and
>> then run regular virtio based drivers (console, tty, block, filesystem,
>> network, ...) along with application specific ones to provide the custom
>> high-level protocols.
> This has more value on the device than on the host, as far as I've
> seen it used (if you want to boot Linux on it and have things
> exposed).
>
> virtio isn't necessarily a match if all you really want is a character
> stream for a console and don't need (or have performance requirements
> beyond what virtio offers) other types of communication.
>
>> This is also similar to what the drivers/pci/endpoint
>> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
>> and of course the rpmsg/remoteproc framework do.
> remoteproc is more about booting a tightly integrated device on an
> embedded system. Also not a match here IMHO.
>
>> In the long run, I would want much more consolidation between the
>> low-level parts of all these frameworks, but moving your high-level
>> protocols to the same virtio method would sound like a step in the
>> direction towards a generialized framework and easier sharing of
>> the abstractions.
> For a simple naive console/character stream, doing something on top of
> hvc might be easier -- it already does polling for you, etc.
>
> Of course, the intent is not to ever use it as a console for the host
> here, so that aspect of hvc isn't useful. But it gives you a bunch of
> other stuff for free with just getchar/putchar interfaces to
> implement.
>
>
> -Olof
Dan Carpenter April 18, 2020, 11:45 a.m. UTC | #10
On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
> > > +static int bcm_vk_dma_alloc(struct device *dev,
> > > +			    struct bcm_vk_dma *dma,
> > > +			    int direction,
> > > +			    struct _vk_data *vkdata)
> > > +{
> > > +	dma_addr_t addr, sg_addr;
> > > +	int err;
> > > +	int i;
> > > +	int offset;
> > > +	uint32_t size;
> > > +	uint32_t remaining_size;
> > > +	uint32_t transfer_size;
> > > +	uint64_t data;
> > > +	unsigned long first, last;
> > > +	struct _vk_data *sgdata;
> > > +
> > > +	/* Get 64-bit user address */
> > > +	data = get_unaligned(&(vkdata->address));
> > Extra parens.
> removed
> > 
> > > +
> > > +	/* offset into first page */
> > > +	offset = offset_in_page(data);
> > > +
> > > +	/* Calculate number of pages */
> > > +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
> > > +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
> > > +	dma->nr_pages = last - first + 1;
> > > +
> > > +	/* Allocate DMA pages */
> > > +	dma->pages = kmalloc_array(dma->nr_pages,
> > > +				   sizeof(struct page *),
> > > +				   GFP_KERNEL);
> > > +	if (dma->pages == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
> > > +		data, vkdata->size, dma->nr_pages);
> > > +
> > > +	dma->direction = direction;
> > > +
> > > +	/* Get user pages into memory */
> > > +	err = get_user_pages_fast(data & PAGE_MASK,
> > > +				  dma->nr_pages,
> > > +				  direction == DMA_FROM_DEVICE,
> > > +				  dma->pages);
> > > +	if (err != dma->nr_pages) {
> > > +		dma->nr_pages = (err >= 0) ? err : 0;
> > > +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
> > > +			err, dma->nr_pages);
> > > +		return err < 0 ? err : -EINVAL;
> > > +	}
> > > +
> > > +	/* Max size of sg list is 1 per mapped page + fields at start */
> > > +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
> > > +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
> > > +
> > > +	/* Allocate sglist */
> > > +	dma->sglist = dma_alloc_coherent(dev,
> > > +					 dma->sglen,
> > > +					 &dma->handle,
> > > +					 GFP_KERNEL);
> > 
> > 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
> > 					 GFP_KERNEL);
> done
> > 
> > 
> > 
> > > +	if (!dma->sglist)
> > > +		return -ENOMEM;
> > No cleanup?
> what needs to be cleaned up?

dma->pages should be freed probably?  And a put_user_pages_fast()?

regards,
dan carpenter
Dan Carpenter April 18, 2020, 11:47 a.m. UTC | #11
On Sat, Apr 18, 2020 at 02:45:16PM +0300, Dan Carpenter wrote:
> On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
> > > > +static int bcm_vk_dma_alloc(struct device *dev,
> > > > +			    struct bcm_vk_dma *dma,
> > > > +			    int direction,
> > > > +			    struct _vk_data *vkdata)
> > > > +{
> > > > +	dma_addr_t addr, sg_addr;
> > > > +	int err;
> > > > +	int i;
> > > > +	int offset;
> > > > +	uint32_t size;
> > > > +	uint32_t remaining_size;
> > > > +	uint32_t transfer_size;
> > > > +	uint64_t data;
> > > > +	unsigned long first, last;
> > > > +	struct _vk_data *sgdata;
> > > > +
> > > > +	/* Get 64-bit user address */
> > > > +	data = get_unaligned(&(vkdata->address));
> > > Extra parens.
> > removed
> > > 
> > > > +
> > > > +	/* offset into first page */
> > > > +	offset = offset_in_page(data);
> > > > +
> > > > +	/* Calculate number of pages */
> > > > +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
> > > > +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
> > > > +	dma->nr_pages = last - first + 1;
> > > > +
> > > > +	/* Allocate DMA pages */
> > > > +	dma->pages = kmalloc_array(dma->nr_pages,
> > > > +				   sizeof(struct page *),
> > > > +				   GFP_KERNEL);
> > > > +	if (dma->pages == NULL)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
> > > > +		data, vkdata->size, dma->nr_pages);
> > > > +
> > > > +	dma->direction = direction;
> > > > +
> > > > +	/* Get user pages into memory */
> > > > +	err = get_user_pages_fast(data & PAGE_MASK,
> > > > +				  dma->nr_pages,
> > > > +				  direction == DMA_FROM_DEVICE,
> > > > +				  dma->pages);
> > > > +	if (err != dma->nr_pages) {
> > > > +		dma->nr_pages = (err >= 0) ? err : 0;
> > > > +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
> > > > +			err, dma->nr_pages);
> > > > +		return err < 0 ? err : -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* Max size of sg list is 1 per mapped page + fields at start */
> > > > +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
> > > > +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
> > > > +
> > > > +	/* Allocate sglist */
> > > > +	dma->sglist = dma_alloc_coherent(dev,
> > > > +					 dma->sglen,
> > > > +					 &dma->handle,
> > > > +					 GFP_KERNEL);
> > > 
> > > 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
> > > 					 GFP_KERNEL);
> > done
> > > 
> > > 
> > > 
> > > > +	if (!dma->sglist)
> > > > +		return -ENOMEM;
> > > No cleanup?
> > what needs to be cleaned up?
> 
> dma->pages should be freed probably?  And a put_user_pages_fast()?

Sorry put_user_pages_fast() isn't a function.  My bad.

regards,
dan carpenter
Scott Branden April 18, 2020, 5:25 p.m. UTC | #12
Thanks Dan, I'll send out new version as soon as my other patch (you had 
requested for test_fx mutex cleanups)
https://lore.kernel.org/linux-arm-msm/20200415002517.4328-1-scott.branden@broadcom.com/

hits the linux-next tree so this patch series will apply cleanly to 
linux-next.




On 2020-04-18 4:47 a.m., Dan Carpenter wrote:
> On Sat, Apr 18, 2020 at 02:45:16PM +0300, Dan Carpenter wrote:
>> On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
>>>>> +static int bcm_vk_dma_alloc(struct device *dev,
>>>>> +			    struct bcm_vk_dma *dma,
>>>>> +			    int direction,
>>>>> +			    struct _vk_data *vkdata)
>>>>> +{
>>>>> +	dma_addr_t addr, sg_addr;
>>>>> +	int err;
>>>>> +	int i;
>>>>> +	int offset;
>>>>> +	uint32_t size;
>>>>> +	uint32_t remaining_size;
>>>>> +	uint32_t transfer_size;
>>>>> +	uint64_t data;
>>>>> +	unsigned long first, last;
>>>>> +	struct _vk_data *sgdata;
>>>>> +
>>>>> +	/* Get 64-bit user address */
>>>>> +	data = get_unaligned(&(vkdata->address));
>>>> Extra parens.
>>> removed
>>>>> +
>>>>> +	/* offset into first page */
>>>>> +	offset = offset_in_page(data);
>>>>> +
>>>>> +	/* Calculate number of pages */
>>>>> +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
>>>>> +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
>>>>> +	dma->nr_pages = last - first + 1;
>>>>> +
>>>>> +	/* Allocate DMA pages */
>>>>> +	dma->pages = kmalloc_array(dma->nr_pages,
>>>>> +				   sizeof(struct page *),
>>>>> +				   GFP_KERNEL);
>>>>> +	if (dma->pages == NULL)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
>>>>> +		data, vkdata->size, dma->nr_pages);
>>>>> +
>>>>> +	dma->direction = direction;
>>>>> +
>>>>> +	/* Get user pages into memory */
>>>>> +	err = get_user_pages_fast(data & PAGE_MASK,
>>>>> +				  dma->nr_pages,
>>>>> +				  direction == DMA_FROM_DEVICE,
>>>>> +				  dma->pages);
>>>>> +	if (err != dma->nr_pages) {
>>>>> +		dma->nr_pages = (err >= 0) ? err : 0;
>>>>> +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
>>>>> +			err, dma->nr_pages);
>>>>> +		return err < 0 ? err : -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	/* Max size of sg list is 1 per mapped page + fields at start */
>>>>> +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
>>>>> +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
>>>>> +
>>>>> +	/* Allocate sglist */
>>>>> +	dma->sglist = dma_alloc_coherent(dev,
>>>>> +					 dma->sglen,
>>>>> +					 &dma->handle,
>>>>> +					 GFP_KERNEL);
>>>> 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
>>>> 					 GFP_KERNEL);
>>> done
>>>>
>>>>> +	if (!dma->sglist)
>>>>> +		return -ENOMEM;
>>>> No cleanup?
>>> what needs to be cleaned up?
>> dma->pages should be freed probably?  And a put_user_pages_fast()?
> Sorry put_user_pages_fast() isn't a function.  My bad.
>
> regards,
> dan carpenter