[v2,0/1] USB: usbfs: replace atomic64 accesses by spinlock

Message

Ingo Rohloff Feb. 9, 2022, 12:33 p.m. UTC

V2: 
Incorporated Alan Sterns review comments: 
Thanks for mentioning the READ_ONCE() semantics; I really had no clue.

Note: 
I think it's also correct to NOT use the "irqsave" variants
of spin_lock_irq/spin_unlock_irq in "usbfs_increase_memory_usage()".

I am not sure if it's worth it?



V1:
patch for f_fs.c:
> > > > + atomic64_sub(amount, &ffs->mmap_mem_usage);      

Greg KH:
> > > Why not use a real lock instead of trying to do a fake one with
> > > this atomic variable?  

Ingo:
> > I just took the code from "drivers/usb/core/devio.c",
> > "usbfs_increase_memory_usage()".
> > ...
> > You are of course right: You can easily use a lock here and this
> > makes the intention of the code a lot clearer I guess.
> > 
> > I will modify the patch accordingly.    

Alan Stern:
> If you also feel like making a similar change to the code in devio.c,
> it would be welcome.  

Ingo Rohloff (1):
  USB: usbfs: Use a spinlock instead of atomic accesses to tally used
    memory.

 drivers/usb/core/devio.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Comments

Alan Stern Feb. 9, 2022, 2:54 p.m. UTC | #1

On Wed, Feb 09, 2022 at 01:33:02PM +0100, Ingo Rohloff wrote:
> V2: 
> Incorporated Alan Sterns review comments: 
> Thanks for mentioning the READ_ONCE() semantics; I really had no clue.
> 
> Note: 
> I think it's also correct to NOT use the "irqsave" variants
> of spin_lock_irq/spin_unlock_irq in "usbfs_increase_memory_usage()".
> 
> I am not sure if it's worth it?

I checked the file, and it does look as though these routines are called 
only in contexts that can sleep.  But using the _irq variants doesn't 
hurt much, so you might as well keep it that way.

Alan Stern