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

[Bhaumik Bhatt]

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