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

[Manivannan Sadhasivam]

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

> > >  	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

> 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