linux-kernel - Re: [PATCH v7 5/5] media: vsp1: Add VSPX support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250424132712.GC18085@pendragon.ideasonboard.com>
Date: Thu, 24 Apr 2025 16:27:12 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Niklas Söderlund <niklas.soderlund@...natech.se>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v7 5/5] media: vsp1: Add VSPX support

On Thu, Apr 24, 2025 at 03:18:23PM +0200, Jacopo Mondi wrote:
> On Thu, Apr 24, 2025 at 12:10:35AM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 01, 2025 at 04:22:05PM +0200, Jacopo Mondi wrote:
> > > Add support for VSPX, a specialized version of the VSP2 that
> > > transfer data to the ISP. The VSPX is composed by two RPF units
> >
> > It seems you forgot to take comments from v2 into account.
> >
> 
> Are you referring to the commit message alone ? Or is there anything
> else ?

Yes, just the commit message.

> > > to read data from external memory and an IIF instance that performs
> > > transfer towards the ISP.
> > >
> 
> [snip]
> 
> > > + *		 buffer CPU-mapped address and the bus address
> > > + *
> > > + * Return: %0 on success or a negative error code on failure
> > > + */
> > > +int vsp1_isp_alloc_buffers(struct device *dev, size_t size,
> > > +			   struct vsp1_isp_buffer_desc *buffer_desc)
> > > +{
> > > +	struct device *bus_master = vsp1_isp_get_bus_master(dev);
> > > +
> > > +	if (IS_ERR_OR_NULL(bus_master))
> > > +		return -ENODEV;
> > > +
> > > +	buffer_desc->cpu_addr = dma_alloc_coherent(bus_master, size,
> > > +						   &buffer_desc->dma_addr,
> > > +						   GFP_KERNEL);
> > > +	if (IS_ERR_OR_NULL(buffer_desc->cpu_addr))
> > > +		return -EINVAL;
> >
> > As commented by Alok, this should be
> >
> > 	if (!buffer_desc->cpu_addr)
> > 		return -ENOMEM;
> 
> Done, thanks Alok for the comment
> 
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vsp1_isp_alloc_buffers);
> >
> > Where is the buffer freed ?
> >
> 
> Right, I presume a call to dma_free_coherent() is needed in the
> vb2 buf_cleanup() operation call path ?

It can be done by the ISP driver (directly, or through a function
exported by the VSP driver, to match vsp1_isp_alloc_buffers()), or be
done by the VSP driver. The important part is to document the design
properly.

-- 
Regards,

Laurent Pinchart