Re: [PATCH] staging: Add Mediatek High Frequency Manager Framework

Re: [PATCH] staging: Add Mediatek High Frequency Manager Framework

[Greg Kroah-Hartman]

On Tue, Aug 04, 2020 at 03:52:49PM +0800, hongxu.zhao wrote:
> Add a new sensor framework into linux kernel which can support multi client request sensor data.
> There are the following features:
>     1.Ringbuffer between manager and client;
>     2.Kernel space user interface;
>     3.User space user interface with syscall;
>     4.Each client hang detect mechanism;
>     5.Polling timer management in framework no need driver concern;
>     6.Polling kthread work intergrated into a single kthread
>       worker to save system resources in framework no need driver concern;
>     7.Proc file system to show manager device and client details;
>     8.Compitable with android and widely used in many mediatek platform products;
> 
> Change-Id: I6361cdc2d51de50f66eede7df099c4575e7ec473

Did you not run checkpatch.pl on this?  :)

No need for change-id here.

But, most importantly, why is this in drivers/staging?  What keeps it
from being in the "real" part of the kernel?  I need a TODO file in the
directory of the driver listing what remains to be done and who is
responsible for doing this work and reviewing patches.

Can you resend this with that file added and the Change-id removed?

Also, why not just use the IIO interface, why are you creating
yet-another api for sensors?  We already have 2, making a third seems
like something that guarantees this will never be mergable to the
correct part of the kernel.

And finally, /proc/ is not for devices, that is what sysfs is for,
please use that.

thanks,

greg k-h

Re: [PATCH] staging: Add Mediatek High Frequency Manager Framework

[Greg Kroah-Hartman]

On Thu, Aug 06, 2020 at 04:32:28PM +0800, hongxu.zhao wrote:
> On Tue, 2020-08-04 at 10:11 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 04, 2020 at 03:52:49PM +0800, hongxu.zhao wrote:
> > > Add a new sensor framework into linux kernel which can support multi client request sensor data.
> > > There are the following features:
> > >     1.Ringbuffer between manager and client;
> > >     2.Kernel space user interface;
> > >     3.User space user interface with syscall;
> > >     4.Each client hang detect mechanism;
> > >     5.Polling timer management in framework no need driver concern;
> > >     6.Polling kthread work intergrated into a single kthread
> > >       worker to save system resources in framework no need driver concern;
> > >     7.Proc file system to show manager device and client details;
> > >     8.Compitable with android and widely used in many mediatek platform products;
> > > 
> > > Change-Id: I6361cdc2d51de50f66eede7df099c4575e7ec473
> > 
> > Did you not run checkpatch.pl on this?  :)
> > 
> > No need for change-id here.
> > 
> > But, most importantly, why is this in drivers/staging?  What keeps it
> > from being in the "real" part of the kernel?  I need a TODO file in the
> > directory of the driver listing what remains to be done and who is
> > responsible for doing this work and reviewing patches.
> > 
> > Can you resend this with that file added and the Change-id removed?
> > 
> > Also, why not just use the IIO interface, why are you creating
> > yet-another api for sensors?  We already have 2, making a third seems
> > like something that guarantees this will never be mergable to the
> > correct part of the kernel.
> > 
> > And finally, /proc/ is not for devices, that is what sysfs is for,
> > please use that.
> 
> I have modified checkpatch issue, but blocked by ARCH=alpha build error
> and I can't reproduce this build error in mediatek environment. I need
> spend some time setting up an environment to solve this problem and will
> send you the latest patch together after solving the problem of alpha
> build error.

If you can't easily fix the alpha issue, you can just mark the driver as
not able to be built there for now.

> Firstly I want keep it in the real part of kernel and I send mail to
> community to find the right maintainer, unfortunately, several emails
> were not answered.

Pointers to those emails on lore.kernel.org?

> Secondly I found iio upstream history it also started from staging at
> the beginning, maybe staging is the best start until it become mature we
> can move it to the real part of kernel.

iio started in staging, but now that the infrastructure is out of it,
there should not be any reason that new drivers be forced to go into
staging too.  You only want to do it this way if for some reason you can
not get the work done on your own.

> Actually, we have already assessed IIO subsystem, but the conclusion is
> that it doesn't meet our requirement:
> 1. iio doesn't have sensor manager in kernel space.

What exactly do you mean by this?  Who needs to be a "manager" here?

> 2. each driver under the iio subsystem needs to create workqueue or
> kthread by itself, waste system resources.

workqueues are very light, how many sensors and what type of resources
are you referring to here?  And adding a whole new user api is not
exactly "tiny" either :)

> 3. iio doesn't have hang detect mechanism to detect polling thread hang.

That's userspace's issue, right?

> We need a sensor manager architecture in kernel space to select the best
> delay and latency that multi-client(user space or kernel space user)
> requested at the same time, and finally dispatch data to each client.

Why does that have to be in the kernel?

> We need lower resource comsumption, each driver can poll data by kthread
> work which intergrated into a single kthread worker to save system
> resources in framework.

Again, how many resources are you really saving here with a whole new
framework?

> We need detect polling thread hang to decide whether to send data to
> him.

Data to where?

> About proc, proc is only for High Frequency Manager Framework to show
> manager details and client details, is not for device drivers.

Then it is not needed :)

> we recommend device driver(like test/test_app.c) use sysfs which under
> High Frequency Manager Framework.

Then you need to document it really well as to what you are doing here.

But again, please try working with the IIO framework as that is what we
have today.  Any improvements you make to it will help everyone out.

Adding a third way to handle sensor data to the kernel is probably not
going to work well for anyone...

thanks,

greg k-h

Re: [PATCH] staging: Add Mediatek High Frequency Manager Framework

[Greg Kroah-Hartman]

On Fri, Aug 07, 2020 at 02:30:29PM +0800, hongxu.zhao wrote:
> On Thu, 2020-08-06 at 12:53 +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 06, 2020 at 04:32:28PM +0800, hongxu.zhao wrote:
> > > On Tue, 2020-08-04 at 10:11 +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Aug 04, 2020 at 03:52:49PM +0800, hongxu.zhao wrote:
> > > > > Add a new sensor framework into linux kernel which can support multi client request sensor data.
> > > > > There are the following features:
> > > > >     1.Ringbuffer between manager and client;
> > > > >     2.Kernel space user interface;
> > > > >     3.User space user interface with syscall;
> > > > >     4.Each client hang detect mechanism;
> > > > >     5.Polling timer management in framework no need driver concern;
> > > > >     6.Polling kthread work intergrated into a single kthread
> > > > >       worker to save system resources in framework no need driver concern;
> > > > >     7.Proc file system to show manager device and client details;
> > > > >     8.Compitable with android and widely used in many mediatek platform products;
> > > > > 
> > > > > Change-Id: I6361cdc2d51de50f66eede7df099c4575e7ec473
> > > > 
> > > > Did you not run checkpatch.pl on this?  :)
> > > > 
> > > > No need for change-id here.
> > > > 
> > > > But, most importantly, why is this in drivers/staging?  What keeps it
> > > > from being in the "real" part of the kernel?  I need a TODO file in the
> > > > directory of the driver listing what remains to be done and who is
> > > > responsible for doing this work and reviewing patches.
> > > > 
> > > > Can you resend this with that file added and the Change-id removed?
> > > > 
> > > > Also, why not just use the IIO interface, why are you creating
> > > > yet-another api for sensors?  We already have 2, making a third seems
> > > > like something that guarantees this will never be mergable to the
> > > > correct part of the kernel.
> > > > 
> > > > And finally, /proc/ is not for devices, that is what sysfs is for,
> > > > please use that.
> > > 
> > > I have modified checkpatch issue, but blocked by ARCH=alpha build error
> > > and I can't reproduce this build error in mediatek environment. I need
> > > spend some time setting up an environment to solve this problem and will
> > > send you the latest patch together after solving the problem of alpha
> > > build error.
> > 
> > If you can't easily fix the alpha issue, you can just mark the driver as
> > not able to be built there for now.
> > 
> > > Firstly I want keep it in the real part of kernel and I send mail to
> > > community to find the right maintainer, unfortunately, several emails
> > > were not answered.
> > 
> > Pointers to those emails on lore.kernel.org?
> 
> I sended mail to linux-kernel@vger.kernel.org

And that was it?  You need to at least cc: people involved so we can see
it, no one can keep up with lkml on its own.

> > > Secondly I found iio upstream history it also started from staging at
> > > the beginning, maybe staging is the best start until it become mature we
> > > can move it to the real part of kernel.
> > 
> > iio started in staging, but now that the infrastructure is out of it,
> > there should not be any reason that new drivers be forced to go into
> > staging too.  You only want to do it this way if for some reason you can
> > not get the work done on your own.
> 
> Yes, I want started in staging and want more sensor driver(vendor
> driver) added to high frequency manager framework.

Again, don't add stuff to staging unless there is a good reason it needs
to be there.

> > > Actually, we have already assessed IIO subsystem, but the conclusion is
> > > that it doesn't meet our requirement:
> > > 1. iio doesn't have sensor manager in kernel space.
> > 
> > What exactly do you mean by this?  Who needs to be a "manager" here?
> 
> >From our experience, mediatek have many platforms and different
> customers have different requirement.
> 
> Firstly customer requirement:
>    Kernel space         |           user space
> ----------------------------------------------------------------
> driver -- framework -- syscall -- client get data in process
> 
> Face this demand, first architecture have no manager service.

Why is a "requirement" divided up this way?  Shouldn't the only
requirement be that a specific userspace program get the needed
information?  Other than that, the architecture below it does not have
to be a specific way, right?

> Secondly customer requirement:
>     Kernel space         |         user space
> ---------------------------------------------------------------------
> driver -- framework -- syscall -- framework service --IPC -- client A
> 
> |--IPC --client B 
> 
> Face this demand, second architecture we add a sensor manager framework
> as service in user space, receive client's enable disable command and
> dispatch data to those clients.
> Then second architecture have a sensor manager in user space and through
> ipc communicate;
> 
> Thirdly customer requirement:
>     Kernel space                 |                 user space
> --------------------------------------------------------------------------
> driver -- framework service -- syscall -- client A get data in process A
>                 |                      |--client B get data in process B
>                 |--client C get data in kernel space

Wait, why would a kernel driver need to get data from a sensor?

And if it does, why can't it just call it directly no matter what the
framework is?  Why can't you do that today with IIO?  Wouldn't it be
much simpler to add that addition to IIO instead of creating a whole new
framework that everyone then has to decide which to add their driver
too?

You don't want a driver author having to choose which interface to
support, they should not need to do that at all.  There should only be
one in-kernel api they need to tie into.  To expect everyone to do
duplicate work is not a valid solution at all.

> Face this demand, sensor manager framework service in user space can't
> meet this requirement, so third architecture we put sensor manager
> framework service to kernel space; Client A and client B through syscall
> communicate with framework service, client c through function call
> communicate with framework service. This is our finally architecture, we
> called high frequency sensor manager framework, high frequency means
> reduce one IPC transfer between framework service and client in user
> space, this architecture will have better performance, and no need
> manager service in user space through ipc communicate between service
> and client.

Again, work with IIO to get the missing pieces added there please.

> > > 2. each driver under the iio subsystem needs to create workqueue or
> > > kthread by itself, waste system resources.
> > 
> > workqueues are very light, how many sensors and what type of resources
> > are you referring to here?  And adding a whole new user api is not
> > exactly "tiny" either :)
> 
> Yes, workqueue are very light, but high frequency sensor (gyro accel and
> so on care latency) can't use system workqueue, must use high prority
> kthread or single thread workqueue.

That's fine, we do that today.

> In android system, there are almost
> 20 - 30 sensor types, one sensor driver user one kthread or single
> thread workqueue maybe not good.:)

In an android system we have 4-8 cores so a single workqueue for each
sensor type is trivial :)

> > > 3. iio doesn't have hang detect mechanism to detect polling thread hang.
> > 
> > That's userspace's issue, right?
> 
> No, in finally architecture, polling thread is not only user space
> issue, kernel space also have polling thread to poll sensor data. When
> sensor generate an event and through manager framework dispatch to
> client A B C, hang detect mechanism will detect which client don't
> response to service then service don't dispatch data to this client.:)

I'm sorry, I don't understand this.  Please work with the IIO developers
to resolve any broken issues.

> > > We need a sensor manager architecture in kernel space to select the best
> > > delay and latency that multi-client(user space or kernel space user)
> > > requested at the same time, and finally dispatch data to each client.
> > 
> > Why does that have to be in the kernel?
> 
> The reason answered before.
> 
> > > We need lower resource comsumption, each driver can poll data by kthread
> > > work which intergrated into a single kthread worker to save system
> > > resources in framework.
> > 
> > Again, how many resources are you really saving here with a whole new
> > framework?
> 
> Assume high frequency manager framework and iio framework the same code
> size, but for drivers, each driver use one  kthread or single thread
> workqueue compare to N drivers use a kthread worker, which is better?:)
> 
> > > We need detect polling thread hang to decide whether to send data to
> > > him.
> > 
> > Data to where?
> 
> Data don't send to polling thread who hanged.
> 
> > > About proc, proc is only for High Frequency Manager Framework to show
> > > manager details and client details, is not for device drivers.
> > 
> > Then it is not needed :)
> > 
> > > we recommend device driver(like test/test_app.c) use sysfs which under
> > > High Frequency Manager Framework.
> > 
> > Then you need to document it really well as to what you are doing here.
> > 
> > But again, please try working with the IIO framework as that is what we
> > have today.  Any improvements you make to it will help everyone out.
> > 
> > Adding a third way to handle sensor data to the kernel is probably not
> > going to work well for anyone...
> 
> Yes, I approve your ideas, but iio framework is too much more
> complicated and widely used, if I improve iio framework maybe not
> compatible with old interface. Then I decide add a new framework to
> staging. :)

Again, I can not accept a new framework that requires each driver that
already is supported in IIO to write a new driver, that would be crazy.

If the IIO developers and maintainer agree with your proposal that there
are problems with IIO and that this is the proper solution, that's fine.
But don't try to route around them without working with them first.
That's just rude :)

thanks,

greg k-h

Re: [PATCH] staging: Add Mediatek High Frequency Manager Framework

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

Hi "hongxu.zhao",

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/hongxu-zhao/staging-Add-Mediatek-High-Frequency-Manager-Framework/20200804-155814
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 5bbd90550da8f7bdac769b5825597e67183c9411
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from drivers/staging/hf_manager/core/hf_manager.c:9:
   drivers/staging/hf_manager/core/hf_manager.c: In function 'hf_manager_io_schedule':
>> include/linux/compiler.h:301:1: error: expected expression before 'do'
     301 | do {         \
         | ^~
   arch/alpha/include/asm/atomic.h:34:27: note: in expansion of macro 'WRITE_ONCE'
      34 | #define atomic64_set(v,i) WRITE_ONCE((v)->counter, (i))
         |                           ^~~~~~~~~~
   drivers/staging/hf_manager/core/hf_manager.h:145:40: note: in expansion of macro 'atomic64_set'
     145 | #define set_interrupt_timestamp(m, t) (atomic64_set(&m->timestamp, t))
         |                                        ^~~~~~~~~~~~
   drivers/staging/hf_manager/core/hf_manager.c:171:2: note: in expansion of macro 'set_interrupt_timestamp'
     171 |  set_interrupt_timestamp(manager, timestamp);
         |  ^~~~~~~~~~~~~~~~~~~~~~~

vim +/do +301 include/linux/compiler.h

a5460b5e5fb826 Will Deacon 2019-12-16  299  
9e343b467c7037 Will Deacon 2019-12-13  300  #define WRITE_ONCE(x, val)						\
9e343b467c7037 Will Deacon 2019-12-13 @301  do {									\
9e343b467c7037 Will Deacon 2019-12-13  302  	compiletime_assert_rwonce_type(x);				\
44b97dccb2291a Marco Elver 2020-05-21  303  	__WRITE_ONCE(x, val);						\
9e343b467c7037 Will Deacon 2019-12-13  304  } while (0)
9e343b467c7037 Will Deacon 2019-12-13  305  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65090 bytes --]