linux-kernel - Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure

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]

Date:   Thu, 15 Jul 2021 13:50:48 +0100
From:   Cristian Marussi <cristian.marussi@....com>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Jassi Brar <jassisinghbrar@...il.com>
Subject: Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information
 into a structure

On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote:
> > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> > > Currently all the PCC channel specific information are stored/maintained
> > > in global individual arrays for each of those information. It is not
> > > scalable and not clean if we have to stash more channel specific
> > > information. Couple of reasons to stash more information are to extend
> > > the support to Type 3/4 PCCT subspace and also to avoid accessing the
> > > PCCT table entries themselves each time we need the information.
> > > 
> > > This patch moves all those PCC channel specific information into a
> > > separate structure pcc_chan_info.
> > > 
> > > Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> > > ---
> > 
> > Hi Sudeep,
> > 
> > >  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
> > >  1 file changed, 53 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 23391e224a68..c5f481a615b0 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -64,12 +64,20 @@
> > >  
> > >  static struct mbox_chan *pcc_mbox_channels;
> > >  
> > > -/* Array of cached virtual address for doorbell registers */
> > > -static void __iomem **pcc_doorbell_vaddr;
> > > -/* Array of cached virtual address for doorbell ack registers */
> > > -static void __iomem **pcc_doorbell_ack_vaddr;
> > > -/* Array of doorbell interrupts */
> > > -static int *pcc_doorbell_irq;
> > > +/**
> > > + * struct pcc_chan_info - PCC channel specific information
> > > + *
> > > + * @db_vaddr: cached virtual address for doorbell register
> > > + * @db_ack_vaddr: cached virtual address for doorbell ack register
> > > + * @db_irq: doorbell interrupt
> > > + */
> > > +struct pcc_chan_info {
> > > +	void __iomem *db_vaddr;
> > > +	void __iomem *db_ack_vaddr;
> > > +	int db_irq;
> > > +};
> >
> > Given that this db_irq represents the optional completion interrupt that is
> > used platform-->OSPM to signal command completions and/or notifications/
> > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
> > Interrupt" and also referred in this driver as such somewherelse, is it not
> > misleading to call it then here "doorbell interrupt" since the "doorbell" in
> > this context is usually the interrupt that goes the other way around
> > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
> > registers ? (that are indeed called Doorbell throughout this driver down below
> > ...but I understand this was the nomenclature used also before in this driver)
> >
> 
> Exactly, I share your thoughts and I completely agree. I didn't want to change
> it in this patch as that would mix 2 different change and makes it hard to
> review. I assume you might have already seen the 8/13 which renames this
> before we add more such registers in later patches.
> 

Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are
consolidated and renamed. So that confused me even more :D

Thanks,
Cristian
> --
> Regards,
> Sudeep