Re: [PATCH v5 07/12] PM / devfreq: export devfreq_class

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Matthias,

On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote:
> Hi,
> 
> On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote:
> 
>> I didn't see any framework which exporting the class instance.
>> It is very dangerous. Unknown device drivers is able to reset
>> the 'devfreq_class' instance. I can't agree this approach.
> 
> While I agree that it is potential dangerous it is actually a common
> practice to export the class:
> 

I tried to find the real usage of exported class instance
and I add the comment for each class instance. Almost exported class
instance are used in the their own director or some exported class
like rio_mport_class/switchtec_class are created from specific device driver
instead of subsystem.

Only following two cases are used on outside of subsystem directory.
devtmpfs.c and alarmtimer.c are core feature of linux kernel.

	drivers/base/devtmpfs.c uses 'block_class'.
	kernel/time/alarmtimer.c uses 'rtc_class'.

I cannot yet agree this approach due to only block_class and rtc_class.

You added the following comment why devfreq_class instance is necessary.
Actullay, I don't know the best solution right now. But, all device drivers
can be added or removed if driver uses the module type. It is not a problem
for only devfreq instance. 

/*
+	 * devfreq devices can be added and removed at runtime, hence they
+	 * must also be handled dynamically. The class_interface notifies us
+	 * whenever a device is added or removed. When the interface is
+	 * registered ci->add_dev() is called for all existing devfreq
+	 * devices.
*/


> grep "extern struct class " include/linux/ -R
> include/linux/rio.h:extern struct class rio_mport_class;
rio_mport_class is created on drivers/rapidio/rio-drivers.c.
It means that just device driver create the 'rio_mport_class' class
instead of any linux kernel subsystem.

> include/linux/tty.h:extern struct class *tty_class;
tty_class is not used on outside of drivers/tty

> include/linux/fb.h:extern struct class *fb_class;
fb_class is not used on outside of drivers/video/fbdev

> include/linux/ide.h:extern struct class *ide_port_class;
ide_port_class is not used on outside of drivers/ide.

> include/linux/device.h:extern struct class * __must_check __class_create(struct module *owner,

> include/linux/devfreq.h:extern struct class *devfreq_class;
not yet

> include/linux/switchtec.h:extern struct class *switchtec_class;
switchtec_class is created on drivers/pci/switch/switchtec.c
and then switchtec_class is only used on drivers/ntb/hw/mscc/ntb_hw_switchtec.c.
It is not subsystem. Just switchtec.c device driver makes the their own class.

> include/linux/input.h:extern struct class input_class;
	input_class is not used on outside of drivers/input.

> include/linux/power_supply.h:extern struct class *power_supply_class;
	power_supply_class is not used on outside of drivers/power/supply.


> include/linux/genhd.h:extern struct class block_class;
	drivers/base/devtmpfs.c uses 'block_class'.

> include/linux/rtc.h:extern struct class *rtc_class;
	kernel/time/alarmtimer.c uses 'rtc_class'.

> 
> struct class_interface and class_interface_register() would be
> pointless without exported classes.
> 
> My understanding is that the kernel is often lax on encapsulation and
> exposes private/delicate data pragmatically within the kernel when
> needed because "the kernel trusts itself".
> 
> Thanks
> 
> Matthias
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics