On Thu, Mar 14, 2024 at 02:54:34PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:06)
> > The variables tracking allocated pages fragments in vchiq_arm.c
> > can be easily moved to struct vchiq_drvdata instead of being global.
> > This helps us to drop the non-essential global variables in the vchiq
> > interface.
> >
> > Drop the TODO item as well, as it is now totally addressed:
> > > Get rid of all non essential global structures and create a proper per
> > device structure
> >
> > No functional changes intended in this patch.
> >
> > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/staging/vc04_services/interface/TODO | 8 ---
> > .../interface/vchiq_arm/vchiq_arm.c | 53 +++++++++----------
> > .../interface/vchiq_arm/vchiq_arm.h | 6 +++
> > 3 files changed, 30 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> > index 05eb5140d096..15f12b8f213e 100644
> > --- a/drivers/staging/vc04_services/interface/TODO
> > +++ b/drivers/staging/vc04_services/interface/TODO
> > @@ -41,14 +41,6 @@ The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
> > indentation deep making it very unpleasant to read. This is specially relevant
> > in the character driver ioctl code and in the core thread functions.
> >
> > -* Get rid of all non essential global structures and create a proper per
> > -device structure
> > -
> > -The first thing one generally sees in a probe function is a memory allocation
> > -for all the device specific data. This structure is then passed all over the
> > -driver. This is good practice since it makes the driver work regardless of the
> > -number of devices probed.
> > -
> > * Clean up Sparse warnings from __user annotations. See
> > vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
> > is never disclosed to userspace.
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 282f83b335d4..666ab73ce0d1 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
> > };
> >
> > static void __iomem *g_regs;
>
> What happens with this one ? Is it essential to be global? Can it be
> part of the device? Who's mapping it? or unmapping?
>
> If we're keeping it global, I'd add a comment...
>
Ugh... If we hadn't updated the TODO list then we could have fixed this
in a follow on patch instead of redoing the series... To be honest, I'd
prefer if it in a separate patch instead of mixed in to patch 5.
regards,
dan carpenter
