Re: [PATCH V2 3/5] DCC: Added the sysfs entries for DCC(Data Capture and Compare) driver

[Stephen Boyd <swboyd@chromium.org>]

Quoting schowdhu@codeaurora.org (2021-04-01 08:42:50)
> On 2021-03-30 01:39, Stephen Boyd wrote:
> > Quoting Souradeep Chowdhury (2021-03-25 01:02:34)
> >> The DCC is a DMA engine designed to store register values either in
> >> case of a system crash or in case of software triggers manually done
> >> by the user.Using DCC hardware and the sysfs interface of the driver
> >> the user can exploit various functionalities of DCC.The user can 
> >> specify
> >> the register addresses,the values of which is stored by DCC in it's
> >> dedicated SRAM.The register addresses can be used either to read from,
> >> write to,first read and store value and then write or to loop.All 
> >> these
> >> options can be exploited using the sysfs interface given to the user.
> >> Following are the sysfs interfaces exposed in DCC driver which are
> >> documented
> >> 1)trigger
> >> 2)config
> >> 3)config_write
> >> 4)config_reset
> >> 5)enable
> >> 6)rd_mod_wr
> >> 7)loop
> >> 
> >> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> >> ---
> >>  Documentation/ABI/testing/sysfs-driver-dcc | 114 
> >> +++++++++++++++++++++++++++++
> > 
> > Please combine this with the driver patch.
> 
> Ack
> 
> > 
> >>  1 file changed, 114 insertions(+)
> >>  create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc
> > 
> > Perhaps this should be an ioctl interface instead of a sysfs interface?
> 
> The reasons for choosing sysfs over ioctl is as follows

Cool, please add these details to the commit text.

> 
> 
> i) As can be seen from the sysfs attribute descriptions, most of it does 
> basic hardware manipulations like dcc_enable, dcc_disable, config reset 
> etc. As a result sysfs is preferred over ioctl as we just need to enter 
> a 0 or 1
> signal in such cases.
> 
> ii) Existing similar debug hardwares are there for which drivers have 
> been written using sysfs interface. One such example is the 
> coresight-etm-trace driver. Following is the link for reference
> 
> https://www.kernel.org/doc/html/latest/trace/coresight/coresight-etm4x-reference.html

I wasn't deeply involved but I recall that the whole coresight sysfs
interface was disliked and it mostly got rewritten to go through the
perf tool instead. Pointing to the coresight sysfs API is not the best
approach here.

Maybe a closer analog would be the watchdog subsystem, which is ioctl
based and uses a character device like /dev/watchdog. Watchdogs are
simple debug features that reboot the system when everything goes wrong.
This looks like a hardware block that can be used to gather information
when the watchdog fires.

Reading the doc closer it is quite frightening that a device like this
can let you read registers in the hardware on-demand and even store
values of registers over time. This is like /dev/mem on steroids. This
needs to be highly restricted as it sounds like it could be used to
snoop on security keys that are set in the hardware or secrets stored in
memory. Is the hardware restricted at all in what it can read?