Re: [PATCH v21 1/2] ACPI:RAS2: Add driver for the ACPI RAS2 feature table

Hi Ashok,

Thanks for looking into this and good catch.
I will incorporate this change in the next version.

Thanks,
Shiju


On Tue, Jul 7, 2026 at 4:47 AM Ashok Raj <[email protected]> wrote:
>
> Hi Shiju
>
> Thanks!
>
> On Mon, Jul 06, 2026 at 01:03:37AM +0100, [email protected] wrote:
> > From: Shiju Jose <[email protected]>
> >
>
> [snip]
>
> > +static int register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int pcc_id)
> > +{
> > +     struct pcc_mbox_chan *pcc_chan;
> > +     struct ras2_sspcc *sspcc;
> > +
> > +     if (pcc_id < 0)
> > +             return -EINVAL;
> > +
> > +     sspcc = ras2_sspcc_get(pcc_id);
> > +     if (sspcc) {
> > +             ras2_ctx->sspcc         = sspcc;
> > +             ras2_ctx->comm_addr     = sspcc->comm_addr;
> > +             ras2_ctx->dev           =
> > +                     sspcc->pcc_chan->mchan->mbox->dev;
> > +             ras2_ctx->pcc_lock      = &sspcc->pcc_lock;
> > +             return 0;
> > +     }
> > +
> > +     sspcc = kzalloc(sizeof(*sspcc), GFP_KERNEL);
> > +     if (!sspcc)
> > +             return -ENOMEM;
> > +
> > +     pcc_chan = pcc_mbox_request_channel(&sspcc->mbox_client, pcc_id);
> > +     if (IS_ERR(pcc_chan)) {
> > +             kfree(sspcc);
> > +             return PTR_ERR(pcc_chan);
> > +     }
> > +
> > +     if (!pcc_chan->shmem) {
> > +             pcc_mbox_free_channel(pcc_chan);
> > +             kfree(sspcc);
> > +             return -EINVAL;
> > +     }
> > +
> > +     sspcc->pcc_id           = pcc_id;
> > +     sspcc->pcc_chan         = pcc_chan;
> > +     sspcc->comm_addr        = pcc_chan->shmem;
> > +     if (pcc_chan->latency)
> > +             sspcc->deadline_us = PCC_NUM_RETRIES * pcc_chan->latency;
> > +     else
> > +             sspcc->deadline_us = PCC_NUM_RETRIES * PCC_CHNL_DEFAULT_LATENCY;
> > +     sspcc->pcc_mrtt         = pcc_chan->min_turnaround_time;
> > +     sspcc->pcc_mpar         = pcc_chan->max_access_rate;
> > +     sspcc->mbox_client.knows_txdone = true;
> > +
>
>   Probably a minor nit ..
>
>   sspcc is published on the global ras2_sspcc list (with a live kref)
>   via list_add() before sspcc->pcc_lock is initialized a few lines
>   later via mutex_init().
>
>   Once list_add() runs, ras2_sspcc_get() can find this sspcc and hand
>   out a pointer to it (kref_get_unless_zero() succeeds since kref_init()
>   already ran). A caller doing so before mutex_init() executes would
>   end up with ras2_ctx->pcc_lock pointing at an uninitialized mutex.
>
>   Currently harmless because the only caller, parse_ras2_table(), walks
>   PCC descriptors strictly sequentially, so no second register_pcc_channel()
>   call for the same pcc_id can land inside the window. But it's relying on
>   that being true rather than the code enforcing it.
>
>   Should we initialize the mutex before publishing the object, e.g.:
>
>         mutex_init(&sspcc->pcc_lock);
>
> > +     kref_init(&sspcc->kref);
> > +
> > +     mutex_lock(&ras2_pcc_list_lock);
> > +     list_add(&sspcc->elem, &ras2_sspcc);
> > +     mutex_unlock(&ras2_pcc_list_lock);
> > +
> > +     ras2_ctx->sspcc         = sspcc;
> > +     ras2_ctx->comm_addr     = sspcc->comm_addr;
> > +     ras2_ctx->dev           = pcc_chan->mchan->mbox->dev;
> > +
>
> > +     mutex_init(&sspcc->pcc_lock); <---------------------
>
> > +     ras2_ctx->pcc_lock      = &sspcc->pcc_lock;
> > +
> > +     return 0;
> > +}
>
> Otherwise
>
> Reviewed-by: Ashok Raj <[email protected]>
>
> Cheers,
> Ashok