linux-kernel - Re: [PATCH v1 1/4] bus: mhi: core: Add support for processing priority of event ring

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

Date:   Fri, 18 Jun 2021 10:43:09 -0700
From:   Bhaumik Bhatt <bbhatt@...eaurora.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, hemantk@...eaurora.org,
        jhugo@...eaurora.org, linux-kernel@...r.kernel.org,
        carl.yin@...ctel.com, naveen.kumar@...ctel.com,
        loic.poulain@...aro.org
Subject: Re: [PATCH v1 1/4] bus: mhi: core: Add support for processing
 priority of event ring

Hi Mani,
On 2021-06-18 10:31 AM, Manivannan Sadhasivam wrote:
> On Fri, Jun 18, 2021 at 10:17:59AM -0700, Bhaumik Bhatt wrote:
>> Hi Mani,
>> 
>> On 2021-06-18 12:03 AM, Manivannan Sadhasivam wrote:
>> > On Thu, Jun 17, 2021 at 02:30:32PM -0700, Bhaumik Bhatt wrote:
>> > > From: Hemant Kumar <hemantk@...eaurora.org>
>> > >
>> > > Event ring priorities are currently set to 1 and are unused.
>> > > Default processing priority for event rings is set to regular
>> > > tasklet. Controllers can choose to use high priority tasklet
>> > > scheduling for certain event rings critical for processing such
>> > > as ones transporting control information if they wish to avoid
>> > > with system scheduling delays for those packets. In order to
>> > > support these use cases, allow controllers to set event ring
>> > > priority to high. This patch only adds support and does not
>> > > enable usage of these priorities.
>> > >
>> > > Signed-off-by: Hemant Kumar <hemantk@...eaurora.org>
>> > > Signed-off-by: Bhaumik Bhatt <bbhatt@...eaurora.org>
>> > > ---
>> > >  drivers/bus/mhi/core/internal.h |  2 +-
>> > >  drivers/bus/mhi/core/main.c     | 19 ++++++++++++++++---
>> > >  include/linux/mhi.h             | 14 ++++++++++++--
>> > >  3 files changed, 29 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/bus/mhi/core/internal.h
>> > > b/drivers/bus/mhi/core/internal.h
>> > > index 672052f..666e102 100644
>> > > --- a/drivers/bus/mhi/core/internal.h
>> > > +++ b/drivers/bus/mhi/core/internal.h
>> > > @@ -535,7 +535,7 @@ struct mhi_event {
>> > >  	u32 intmod;
>> > >  	u32 irq;
>> > >  	int chan; /* this event ring is dedicated to a channel (optional) */
>> > > -	u32 priority;
>> > > +	enum mhi_er_priority priority;
>> >
>> > Instead of using enum for priorities, can we just make use of the
>> > existing "priority" field? Since the existing controllers set it to "1",
>> > can we use "0" as the high priority?
>> >
>> > This way we don't need to change the controller drivers.
>> >
>> I agree but the reasons to do the enum approach was to allow for 
>> future
>> expansion of the handling if it becomes necessary and provide clarity 
>> for
>> the field.
>> 
>> I can always do it this way for now and we can have the enum for 
>> another
>> time but would prefer updating what we have now.
> 
> Yeah, let's deal with it later once the necessity arises.
> 
Sure. I will make the v2.

>> > >  	enum mhi_er_data_type data_type;
>> > >  	struct mhi_ring ring;
>> > >  	struct db_cfg db_cfg;
>> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> > > index 8ac73f9..bfc9776 100644
>> > > --- a/drivers/bus/mhi/core/main.c
>> > > +++ b/drivers/bus/mhi/core/main.c
>> > > @@ -425,10 +425,11 @@ void mhi_create_devices(struct mhi_controller
>> > > *mhi_cntrl)
>> > >  	}
>> > >  }
>> > >
> 
> [...]
> 
>> Existing controllers would be fine.
>> 
>> Do you think we have a problem if a new controller blindly inputs a 
>> "0" in
>> the priority not knowing the impact of it?
>> 
> 
> We should document it in the kernel doc for the struct field and that
> should be enough. We can't do much if people doesn't read the doc ;)
> 
> Thanks,
> Mani
> 
Totally agree :)

>> If you give me a go ahead, I can make these changes in v2 and leave 
>> the enum
>> stuff out.
>> 
>> Thanks,
>> Bhaumik
>> ---
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project