diff mbox series

[11/15] media: atomisp: Replace atomisp_drvfs attr with using driver.dev_groups attr

Message ID 20231231103057.35837-12-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series media: atomisp: NULL pointer deref + missing firmware fixes | expand

Commit Message

Hans de Goede Dec. 31, 2023, 10:30 a.m. UTC 
sysfs attributes preferably should not be manually be registered but
instead the driver.groups / driver.dev_groups driver struct members
should be used to have the driver core handle this in a race free
manner.

Using driver.groups would be the most direct replacement for
driver_[add|remove]_file, but some of the attributes actually need access
to the struct atomisp_device (*), so as part of modernizing this part of
the atomisp driver this change also makes the sysfs attribute device
attributes instead of driver attributes.

*) Before this change accessing these attributes without the driver having
bound would result in a NULL pointer deref, this commit fixes this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_drvfs.c | 148 +++++++-----------
 .../staging/media/atomisp/pci/atomisp_drvfs.h |   5 +-
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   9 +-
 3 files changed, 62 insertions(+), 100 deletions(-)

Comments

Andy Shevchenko Jan. 2, 2024, 12:33 a.m. UTC | #1 
On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> sysfs attributes preferably should not be manually be registered but
> instead the driver.groups / driver.dev_groups driver struct members
> should be used to have the driver core handle this in a race free
> manner.
>
> Using driver.groups would be the most direct replacement for
> driver_[add|remove]_file, but some of the attributes actually need access

..._file()

> to the struct atomisp_device (*), so as part of modernizing this part of
> the atomisp driver this change also makes the sysfs attribute device
> attributes instead of driver attributes.
>
> *) Before this change accessing these attributes without the driver having
> bound would result in a NULL pointer deref, this commit fixes this.

...

> +       if (dbglvl < 1 || dbglvl > 9)

in_range() ?

>                 return -ERANGE;

...

> +static const struct attribute_group dbg_attr_group = {
> +       .attrs = dbg_attrs,
> +};
>
> +const struct attribute_group *dbg_attr_groups[] = {
> +       &dbg_attr_group,
> +       NULL
> +};

ATTRIBUTE_GROUPS()

...

> +#include <linux/sysfs.h>

But why? You can use forward declaration, no?

> +extern const struct attribute_group *dbg_attr_groups[];
Hans de Goede Jan. 2, 2024, 11:30 a.m. UTC | #2 
Hi Andy,

Thank you for all the reviews.

I don't see anything really problematic in your review,
so I hope that Mauro will honor my pull-request and
then I'll fix the small remarks in some follow-up patches.

One remark regarding your review of this patch below:

On 1/2/24 01:33, Andy Shevchenko wrote:
> On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> sysfs attributes preferably should not be manually be registered but
>> instead the driver.groups / driver.dev_groups driver struct members
>> should be used to have the driver core handle this in a race free
>> manner.
>>
>> Using driver.groups would be the most direct replacement for
>> driver_[add|remove]_file, but some of the attributes actually need access
> 
> ..._file()
> 
>> to the struct atomisp_device (*), so as part of modernizing this part of
>> the atomisp driver this change also makes the sysfs attribute device
>> attributes instead of driver attributes.
>>
>> *) Before this change accessing these attributes without the driver having
>> bound would result in a NULL pointer deref, this commit fixes this.
> 
> ...
> 
>> +       if (dbglvl < 1 || dbglvl > 9)
> 
> in_range() ?

ack.

> 
>>                 return -ERANGE;
> 
> ...
> 
>> +static const struct attribute_group dbg_attr_group = {
>> +       .attrs = dbg_attrs,
>> +};
>>
>> +const struct attribute_group *dbg_attr_groups[] = {
>> +       &dbg_attr_group,
>> +       NULL
>> +};
> 
> ATTRIBUTE_GROUPS()

I deliberately wrote this out (had to write this out)
instead of using ATTRIBUTE_GROUPS() because ATTRIBUTE_GROUPS()
makes the groups variable static and here it gets used
in another file then where it is declared.

> 
> ...
> 
>> +#include <linux/sysfs.h>
> 
> But why? You can use forward declaration, no?

True, I'll fix this up in a follow-up patch.

> 
>> +extern const struct attribute_group *dbg_attr_groups[];
> 


Regards,

Hans
Andy Shevchenko Jan. 2, 2024, 9:23 p.m. UTC | #3 
On Tue, Jan 2, 2024 at 1:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/2/24 01:33, Andy Shevchenko wrote:
> > On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +static const struct attribute_group dbg_attr_group = {
> >> +       .attrs = dbg_attrs,
> >> +};
> >>
> >> +const struct attribute_group *dbg_attr_groups[] = {
> >> +       &dbg_attr_group,
> >> +       NULL
> >> +};
> >
> > ATTRIBUTE_GROUPS()
>
> I deliberately wrote this out (had to write this out)
> instead of using ATTRIBUTE_GROUPS() because ATTRIBUTE_GROUPS()
> makes the groups variable static and here it gets used
> in another file then where it is declared.

I see, but can it be refactored / does it make sense to refactor that
it becomes visible only in one module?
Hans de Goede Jan. 17, 2024, 3:03 p.m. UTC | #4 
Hi Andy,

On 1/2/24 22:23, Andy Shevchenko wrote:
> On Tue, Jan 2, 2024 at 1:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/2/24 01:33, Andy Shevchenko wrote:
>>> On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> +static const struct attribute_group dbg_attr_group = {
>>>> +       .attrs = dbg_attrs,
>>>> +};
>>>>
>>>> +const struct attribute_group *dbg_attr_groups[] = {
>>>> +       &dbg_attr_group,
>>>> +       NULL
>>>> +};
>>>
>>> ATTRIBUTE_GROUPS()
>>
>> I deliberately wrote this out (had to write this out)
>> instead of using ATTRIBUTE_GROUPS() because ATTRIBUTE_GROUPS()
>> makes the groups variable static and here it gets used
>> in another file then where it is declared.
> 
> I see, but can it be refactored / does it make sense to refactor that
> it becomes visible only in one module?

The problem is that all the sysfs attr handling code lives
in its own .c file, where as the driver struct is in another
.c file, so we really need to access the list of
attribute-groups from another file then where the list is
declared, which makes the ATTRIBUTE_GROUPS() macro
unsuitable.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/atomisp_drvfs.c b/drivers/staging/media/atomisp/pci/atomisp_drvfs.c
index 1df534bf54d3..293171da1266 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_drvfs.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_drvfs.c
@@ -27,31 +27,17 @@ 
 #include "hmm/hmm.h"
 #include "ia_css_debug.h"
 
+#define OPTION_BIN_LIST			BIT(0)
+#define OPTION_BIN_RUN			BIT(1)
+#define OPTION_VALID			(OPTION_BIN_LIST | OPTION_BIN_RUN)
+
 /*
- * _iunit_debug:
- * dbglvl: iunit css driver trace level
  * dbgopt: iunit debug option:
  *        bit 0: binary list
  *        bit 1: running binary
  *        bit 2: memory statistic
-*/
-struct _iunit_debug {
-	struct device_driver	*drv;
-	struct atomisp_device	*isp;
-	unsigned int		dbglvl;
-	unsigned int		dbgfun;
-	unsigned int		dbgopt;
-};
-
-#define OPTION_BIN_LIST			BIT(0)
-#define OPTION_BIN_RUN			BIT(1)
-#define OPTION_VALID			(OPTION_BIN_LIST \
-					| OPTION_BIN_RUN)
-
-static struct _iunit_debug iunit_debug = {
-	.dbglvl = 0,
-	.dbgopt = OPTION_BIN_LIST,
-};
+ */
+unsigned int dbgopt = OPTION_BIN_LIST;
 
 static inline int iunit_dump_dbgopt(struct atomisp_device *isp,
 				    unsigned int opt)
@@ -88,34 +74,44 @@  static inline int iunit_dump_dbgopt(struct atomisp_device *isp,
 	return ret;
 }
 
-static ssize_t iunit_dbglvl_show(struct device_driver *drv, char *buf)
+static ssize_t dbglvl_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
-	iunit_debug.dbglvl = dbg_level;
-	return sysfs_emit(buf, "dtrace level:%u\n", iunit_debug.dbglvl);
+	unsigned int dbglvl = ia_css_debug_get_dtrace_level();
+
+	return sysfs_emit(buf, "dtrace level:%u\n", dbglvl);
 }
 
-static ssize_t iunit_dbglvl_store(struct device_driver *drv, const char *buf,
-				  size_t size)
+static ssize_t dbglvl_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t size)
 {
-	if (kstrtouint(buf, 10, &iunit_debug.dbglvl)
-	    || iunit_debug.dbglvl < 1
-	    || iunit_debug.dbglvl > 9) {
+	unsigned int dbglvl;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &dbglvl);
+	if (ret)
+		return ret;
+
+	if (dbglvl < 1 || dbglvl > 9)
 		return -ERANGE;
-	}
-	ia_css_debug_set_dtrace_level(iunit_debug.dbglvl);
 
+	ia_css_debug_set_dtrace_level(dbglvl);
 	return size;
 }
+static DEVICE_ATTR_RW(dbglvl);
 
-static ssize_t iunit_dbgfun_show(struct device_driver *drv, char *buf)
+static ssize_t dbgfun_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
-	iunit_debug.dbgfun = atomisp_get_css_dbgfunc();
-	return sysfs_emit(buf, "dbgfun opt:%u\n", iunit_debug.dbgfun);
+	unsigned int dbgfun = atomisp_get_css_dbgfunc();
+
+	return sysfs_emit(buf, "dbgfun opt:%u\n", dbgfun);
 }
 
-static ssize_t iunit_dbgfun_store(struct device_driver *drv, const char *buf,
-				  size_t size)
+static ssize_t dbgfun_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t size)
 {
+	struct atomisp_device *isp = dev_get_drvdata(dev);
 	unsigned int opt;
 	int ret;
 
@@ -123,23 +119,20 @@  static ssize_t iunit_dbgfun_store(struct device_driver *drv, const char *buf,
 	if (ret)
 		return ret;
 
-	ret = atomisp_set_css_dbgfunc(iunit_debug.isp, opt);
-	if (ret)
-		return ret;
+	return atomisp_set_css_dbgfunc(isp, opt);
+}
+static DEVICE_ATTR_RW(dbgfun);
 
-	iunit_debug.dbgfun = opt;
-
-	return size;
+static ssize_t dbgopt_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	return sysfs_emit(buf, "option:0x%x\n", dbgopt);
 }
 
-static ssize_t iunit_dbgopt_show(struct device_driver *drv, char *buf)
-{
-	return sysfs_emit(buf, "option:0x%x\n", iunit_debug.dbgopt);
-}
-
-static ssize_t iunit_dbgopt_store(struct device_driver *drv, const char *buf,
-				  size_t size)
+static ssize_t dbgopt_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t size)
 {
+	struct atomisp_device *isp = dev_get_drvdata(dev);
 	unsigned int opt;
 	int ret;
 
@@ -147,56 +140,27 @@  static ssize_t iunit_dbgopt_store(struct device_driver *drv, const char *buf,
 	if (ret)
 		return ret;
 
-	iunit_debug.dbgopt = opt;
-	ret = iunit_dump_dbgopt(iunit_debug.isp, iunit_debug.dbgopt);
+	dbgopt = opt;
+	ret = iunit_dump_dbgopt(isp, dbgopt);
 	if (ret)
 		return ret;
 
 	return size;
 }
+static DEVICE_ATTR_RW(dbgopt);
 
-static const struct driver_attribute iunit_drvfs_attrs[] = {
-	__ATTR(dbglvl, 0644, iunit_dbglvl_show, iunit_dbglvl_store),
-	__ATTR(dbgfun, 0644, iunit_dbgfun_show, iunit_dbgfun_store),
-	__ATTR(dbgopt, 0644, iunit_dbgopt_show, iunit_dbgopt_store),
+static struct attribute *dbg_attrs[] = {
+	&dev_attr_dbglvl.attr,
+	&dev_attr_dbgfun.attr,
+	&dev_attr_dbgopt.attr,
+	NULL
 };
 
-static int iunit_drvfs_create_files(struct device_driver *drv)
-{
-	int i, ret = 0;
+static const struct attribute_group dbg_attr_group = {
+	.attrs = dbg_attrs,
+};
 
-	for (i = 0; i < ARRAY_SIZE(iunit_drvfs_attrs); i++)
-		ret |= driver_create_file(drv, &iunit_drvfs_attrs[i]);
-
-	return ret;
-}
-
-static void iunit_drvfs_remove_files(struct device_driver *drv)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(iunit_drvfs_attrs); i++)
-		driver_remove_file(drv, &iunit_drvfs_attrs[i]);
-}
-
-int atomisp_drvfs_init(struct atomisp_device *isp)
-{
-	struct device_driver *drv = isp->dev->driver;
-	int ret;
-
-	iunit_debug.isp = isp;
-	iunit_debug.drv = drv;
-
-	ret = iunit_drvfs_create_files(iunit_debug.drv);
-	if (ret) {
-		dev_err(isp->dev, "drvfs_create_files error: %d\n", ret);
-		iunit_drvfs_remove_files(iunit_debug.drv);
-	}
-
-	return ret;
-}
-
-void atomisp_drvfs_exit(void)
-{
-	iunit_drvfs_remove_files(iunit_debug.drv);
-}
+const struct attribute_group *dbg_attr_groups[] = {
+	&dbg_attr_group,
+	NULL
+};
diff --git a/drivers/staging/media/atomisp/pci/atomisp_drvfs.h b/drivers/staging/media/atomisp/pci/atomisp_drvfs.h
index 8f4cc722b881..8495cc133c06 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_drvfs.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_drvfs.h
@@ -19,7 +19,8 @@ 
 #ifndef	__ATOMISP_DRVFS_H__
 #define	__ATOMISP_DRVFS_H__
 
-int atomisp_drvfs_init(struct atomisp_device *isp);
-void atomisp_drvfs_exit(void);
+#include <linux/sysfs.h>
+
+extern const struct attribute_group *dbg_attr_groups[];
 
 #endif /* __ATOMISP_DRVFS_H__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 6e8c9add35f9..0c78c1d82659 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1468,8 +1468,6 @@  static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
 		goto error_unload_firmware;
 	}
 
-	atomisp_drvfs_init(isp);
-
 	return 0;
 
 error_unload_firmware:
@@ -1497,8 +1495,6 @@  static void atomisp_pci_remove(struct pci_dev *pdev)
 {
 	struct atomisp_device *isp = pci_get_drvdata(pdev);
 
-	atomisp_drvfs_exit();
-
 	pm_runtime_get_sync(&pdev->dev);
 	pm_runtime_forbid(&pdev->dev);
 	dev_pm_domain_set(&pdev->dev, NULL);
@@ -1529,11 +1525,12 @@  static const struct pci_device_id atomisp_pci_tbl[] = {
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, ATOMISP_PCI_DEVICE_SOC_CHT)},
 	{0,}
 };
-
 MODULE_DEVICE_TABLE(pci, atomisp_pci_tbl);
 
-
 static struct pci_driver atomisp_pci_driver = {
+	.driver = {
+		.dev_groups = dbg_attr_groups,
+	},
 	.name = "atomisp-isp2",
 	.id_table = atomisp_pci_tbl,
 	.probe = atomisp_pci_probe,