linux-kernel - Re: [PATCH 2/2] staging: media: atomisp: unify initialization flag usage in HMM

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGn2d8MetzWM0bwhdnocP1kLCYcYgmzb1ggPC5Fuv6cXFDniug@mail.gmail.com>
Date: Tue, 8 Jul 2025 17:54:49 +0300
From: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
To: Hans de Goede <hansg@...nel.org>
Cc: mchehab@...nel.org, sakari.ailus@...ux.intel.org, andy@...nel.org, 
	gregkh@...uxfoundation.org, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev, 
	linux-kernel-mentees@...ts.linux.dev, skhan@...uxfoundation.org, 
	dan.carpenter@...aro.org
Subject: Re: [PATCH 2/2] staging: media: atomisp: unify initialization flag
 usage in HMM

Hi Hans,
Thanks for the feedback!

On Mon, Jul 7, 2025 at 5:14 PM Hans de Goede <hansg@...nel.org> wrote:
>
> Hi Abdelrahman,
>
> On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> > Previously, the initialization state of the `hmm_bo_device` was tracked
> > in two places: a global `hmm_initialized` boolean in `hmm.c`, and a local
> > integer `flag` in the `hmm_bo_device` struct. This was redundant and could
> > lead to inconsistent state checks.
> >
> > - Removes the global `hmm_initialized` variable and all checks against it.
> > - Replaces the `int flag` in `struct hmm_bo_device` with a strongly-typed
> >  `enum hmm_bo_device_init_flag flag` (values: UNINITED = 0, INITED = 1).
> > - Initializes `flag` to `HMM_BO_DEVICE_UNINITED` at declaration to
> >   ensure a well-defined starting state.
> > - Removes a redundant `hmm_init()` call inside `__hmm_alloc()` since its
> >   always called after hmm_init()
> >
> > This change improves type safety, consistency, and readability when
> > handling the HMM initialization state.
> >
> > Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
> > ---
> >  .../staging/media/atomisp/include/hmm/hmm_bo.h   |  9 +++++++--
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c      | 16 ++++------------
> >  2 files changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > index e09ac29ac43d..155f9d89b365 100644
> > --- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > +++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > @@ -58,7 +58,10 @@
> >  #define      ISP_VM_SIZE     (0x7FFFFFFF)    /* 2G address space */
> >  #define      ISP_PTR_NULL    NULL
> >
> > -#define      HMM_BO_DEVICE_INITED    0x1
> > +enum hmm_bo_device_init_flag {
> > +     HMM_BO_DEVICE_INITED    = 0x1,
> > +     HMM_BO_DEVICE_UNINITED  = 0x2,
> > +};
> >
> >  enum hmm_bo_type {
> >       HMM_BO_PRIVATE,
> > @@ -86,7 +89,9 @@ struct hmm_bo_device {
> >
> >       /* list lock is used to protect the entire_bo_list */
> >       spinlock_t      list_lock;
> > -     int flag;
> > +
> > +     /* flag to indicate whether the bo device is inited or not */
> > +     enum hmm_bo_device_init_flag flag;
>
> Please just replace this with a "bool initialized"; data
> member taking `true` and `false as values instead of
> introducing a new type for this.
>
Okay , will do this.

> >
> >       /* linked list for entire buffer object */
> >       struct list_head entire_bo_list;
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > index c2ee9d2ec0d5..767a3a24f8e5 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > @@ -24,9 +24,10 @@
> >  #include "mmu/isp_mmu.h"
> >  #include "mmu/sh_mmu_mrfld.h"
> >
> > -struct hmm_bo_device bo_device;
> > +struct hmm_bo_device bo_device = {
> > +     .flag = HMM_BO_DEVICE_UNINITED,
> > +};
> >  static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
> > -static bool hmm_initialized;
> >
> >  int hmm_init(void)
> >  {
> > @@ -38,8 +39,6 @@ int hmm_init(void)
> >               dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
> >               return ret;
> >
> > -     hmm_initialized = true;
> > -
> >       /*
> >        * As hmm use NULL to indicate invalid ISP virtual address,
> >        * and ISP_VM_START is defined to 0 too, so we allocate
> > @@ -62,7 +61,7 @@ void hmm_cleanup(void)
> >       dummy_ptr = 0;
> >
> >       hmm_bo_device_exit(&bo_device);
> > -     hmm_initialized = false;
> > +     bo_device.flag = HMM_BO_DEVICE_UNINITED;
>
> This clearing of the flag / setting `initialized = false` belongs inside bo_exit()
> not here.
>
okay, moving it in v2
>
> >  }
> >
> >  static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
> > @@ -72,13 +71,6 @@ static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
> >       struct hmm_buffer_object *bo;
> >       int ret;
> >
> > -     /*
> > -      * Check if we are initialized. In the ideal world we wouldn't need
> > -      * this but we can tackle it once the driver is a lot cleaner
> > -      */
> > -
> > -     if (!hmm_initialized)
> > -             hmm_init();
> >       /* Get page number from size */
> >       pgnr = size_to_pgnr_ceil(bytes);
> >
>
>
> Regards,
>
> Hans
>
>