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

[Bhaumik Bhatt <bbhatt@codeaurora.org>]

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@codeaurora.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@codeaurora.org>
>> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.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