Re: [PATCH 3/3] usb: gadget: f_fs: Introduce rw_proxy file descriptors

[Neill Kapron <nkapron@google.com>]

On Mon, Jun 15, 2026 at 04:35:39AM +0200, Greg KH wrote:
> On Sun, Jun 14, 2026 at 06:10:02PM +0000, Neill Kapron wrote:
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index 4c1bafb3eef5..0ccfdcfb1810 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -159,7 +159,9 @@ struct ffs_epfile {
> >  	struct mutex			mutex;
> >  
> >  	struct ffs_data			*ffs;
> > -	struct ffs_ep			*ep;	/* P: ffs->eps_lock */
> > +	struct ffs_ep			*ep;		/* P: ffs->eps_lock */
> > +	struct ffs_epfile		*epfile_in;	/* P: ffs->eps_lock */
> > +	struct ffs_epfile		*epfile_out;	/* P: ffs->eps_lock */
> >  
> >  	/*
> >  	 * Buffer for holding data from partial reads which may happen since
> > @@ -219,17 +221,20 @@ struct ffs_epfile {
> >  	struct ffs_buffer		*read_buffer;
> >  #define READ_BUFFER_DROP ((struct ffs_buffer *)ERR_PTR(-ESHUTDOWN))
> >  
> > -	char				name[5];
> > +	char				name[10];
> 
> Why change the size?  Shouldn't that be a separate patch?

The size change is to handle the new proxy file, in the format of
'epXX_rw' (with a null terminator). The size of 10 is a remnant of an
earlier version of this patch which had a slightly different naming
scheme. I will update v2 with name[8] to which will properly reflect the
necessary size required.

> >  	atomic_t			seqno;
> > +
> > +	int				opened_count; /* P: ffs->eps_lock */
> 
> Attempting to track "is this file open or not" almost always fails
> horribly.  Think about file descriptors that can be dup() and passed
> around, the kernel has no idea what is going on with them, nor does it
> have to.
> 
> Yes, we do track if the file is opened or not already, but I'd argue
> that too is broken and should probably be removed and just use the
> normal file descriptor logic instead.
> 

Ack, responded below.

> 
> > @@ -1378,8 +1393,18 @@ ffs_epfile_release(struct inode *inode, struct file *file)
> >  
> >  	mutex_unlock(&epfile->dmabufs_mutex);
> >  
> > -	__ffs_epfile_read_buffer_free(epfile);
> > -	ffs_data_closed(epfile->ffs);
> > +	spin_lock_irq(&ffs->eps_lock);
> > +	if (epfile->is_rw_proxy) {
> > +		epfile->epfile_in->opened_count--;
> > +		if (--epfile->epfile_out->opened_count == 0)
> > +			__ffs_epfile_read_buffer_free(epfile->epfile_out);
> > +	} else {
> > +		if (--epfile->opened_count == 0)
> > +			__ffs_epfile_read_buffer_free(epfile);
> 
> If you drop the opened_count, shouldn't these buffers just get freed
> when the structure themselves get freed?  You are treating the count as
> a "reference counted structure" in a hand-rolled way that might not
> really be right here as it's kind of hard to prove.
> 
> Either use a real reference count for the whole structure (i.e. kref)
> because you need to, or just tie the lifetime of the buffer to the
> larger structure itself.  Otherwise these fake references are going to
> be a pain to track that all is correct with them...
> 

I was concerned about the implementation of opened_count, and was
pursuing a proper kref approach, but it became a somewhat invasive patch
and I didn't want to pollute this patchset with said change, as I didn't
want to potentially introduce issues for users which did not need this
series.

Changing the buffer lifetime to match the larger structure is
straightforward and clean. I will implement that in v2 of this series,
as it fixes an existing issue.

Thanks for the suggestions,
Neill