The driver core supports the ability to handle the creation and removal
of device-specific sysfs files in a race-free manner. Moreover, it can
guarantee the success of creation. Therefore, it should be better to
convert to use dev_groups.
Signed-off-by: Jiasheng Jiang <[email protected]>
---
Changelog:
v2 -> v3:
1. Add is_visible to filter the unneeded files.
v1 -> v2:
1. Change the title.
2. Switch to use an attribute group.
---
drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
1 file changed, 51 insertions(+), 50 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index d5a3986dfee7..479c3a5caaf8 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
static u32 ftdi_232bm_baud_to_divisor(int baud);
static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
static u32 ftdi_2232h_baud_to_divisor(int baud);
+static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int idx);
+static ssize_t latency_timer_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *valbuf, size_t count);
+static ssize_t event_char_store(struct device *dev,
+ struct device_attribute *attr, const char *valbuf, size_t count);
+static ssize_t latency_timer_show(struct device *dev,
+ struct device_attribute *attr, char *buf);
+
+static DEVICE_ATTR_RW(latency_timer);
+static DEVICE_ATTR_WO(event_char);
+
+static struct attribute *ftdi_sio_attrs[] = {
+ &dev_attr_event_char.attr,
+ &dev_attr_latency_timer.attr,
+ NULL,
+};
+
+static const struct attribute_group ftdi_sio_group = {
+ .attrs = ftdi_sio_attrs,
+ .is_visible = ftdi_sio_attr_is_visible,
+};
+
+static const struct attribute_group *ftdi_sio_groups[] = {
+ &ftdi_sio_group,
+ NULL
+};
static struct usb_serial_driver ftdi_sio_device = {
.driver = {
.owner = THIS_MODULE,
.name = "ftdi_sio",
+ .dev_groups = ftdi_sio_groups,
},
.description = "FTDI USB Serial Device",
.id_table = id_table_combined,
@@ -1696,7 +1725,6 @@ static ssize_t latency_timer_store(struct device *dev,
return -EIO;
return count;
}
-static DEVICE_ATTR_RW(latency_timer);
/* Write an event character directly to the FTDI register. The ASCII
value is in the low 8 bits, with the enable bit in the 9th bit. */
@@ -1727,52 +1755,6 @@ static ssize_t event_char_store(struct device *dev,
return count;
}
-static DEVICE_ATTR_WO(event_char);
-
-static int create_sysfs_attrs(struct usb_serial_port *port)
-{
- struct ftdi_private *priv = usb_get_serial_port_data(port);
- int retval = 0;
-
- /* XXX I've no idea if the original SIO supports the event_char
- * sysfs parameter, so I'm playing it safe. */
- if (priv->chip_type != SIO) {
- dev_dbg(&port->dev, "sysfs attributes for %s\n", ftdi_chip_name[priv->chip_type]);
- retval = device_create_file(&port->dev, &dev_attr_event_char);
- if ((!retval) &&
- (priv->chip_type == FT232BM ||
- priv->chip_type == FT2232C ||
- priv->chip_type == FT232RL ||
- priv->chip_type == FT2232H ||
- priv->chip_type == FT4232H ||
- priv->chip_type == FT232H ||
- priv->chip_type == FTX)) {
- retval = device_create_file(&port->dev,
- &dev_attr_latency_timer);
- }
- }
- return retval;
-}
-
-static void remove_sysfs_attrs(struct usb_serial_port *port)
-{
- struct ftdi_private *priv = usb_get_serial_port_data(port);
-
- /* XXX see create_sysfs_attrs */
- if (priv->chip_type != SIO) {
- device_remove_file(&port->dev, &dev_attr_event_char);
- if (priv->chip_type == FT232BM ||
- priv->chip_type == FT2232C ||
- priv->chip_type == FT232RL ||
- priv->chip_type == FT2232H ||
- priv->chip_type == FT4232H ||
- priv->chip_type == FT232H ||
- priv->chip_type == FTX) {
- device_remove_file(&port->dev, &dev_attr_latency_timer);
- }
- }
-
-}
#ifdef CONFIG_GPIOLIB
@@ -2251,7 +2233,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
if (read_latency_timer(port) < 0)
priv->latency = 16;
write_latency_timer(port);
- create_sysfs_attrs(port);
result = ftdi_gpio_init(port);
if (result < 0) {
@@ -2377,8 +2358,6 @@ static void ftdi_sio_port_remove(struct usb_serial_port *port)
ftdi_gpio_remove(port);
- remove_sysfs_attrs(port);
-
kfree(priv);
}
@@ -2915,6 +2894,28 @@ static int ftdi_ioctl(struct tty_struct *tty,
return -ENOIOCTLCMD;
}
+static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int idx)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct usb_serial_port *port = container_of(dev, struct usb_serial_port, dev);
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ umode_t mode = attr->mode;
+
+ if (attr == &dev_attr_latency_timer.attr) {
+ if (priv->chip_type == FT232BM ||
+ priv->chip_type == FT2232C ||
+ priv->chip_type == FT232RL ||
+ priv->chip_type == FT2232H ||
+ priv->chip_type == FT4232H ||
+ priv->chip_type == FT232H ||
+ priv->chip_type == FTX) {
+ return mode;
+ }
+ }
+ return 0;
+}
+
module_usb_serial_driver(serial_drivers, id_table_combined);
MODULE_AUTHOR(DRIVER_AUTHOR);
--
2.25.1
On Fri, Sep 02, 2022 at 05:44:23PM +0800, Jiasheng Jiang wrote:
> The driver core supports the ability to handle the creation and removal
> of device-specific sysfs files in a race-free manner. Moreover, it can
> guarantee the success of creation. Therefore, it should be better to
> convert to use dev_groups.
>
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> Changelog:
>
> v2 -> v3:
>
> 1. Add is_visible to filter the unneeded files.
>
> v1 -> v2:
>
> 1. Change the title.
> 2. Switch to use an attribute group.
> ---
> drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
> 1 file changed, 51 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index d5a3986dfee7..479c3a5caaf8 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
> static u32 ftdi_232bm_baud_to_divisor(int baud);
> static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
> static u32 ftdi_2232h_baud_to_divisor(int baud);
> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
> + struct attribute *attr, int idx);
> +static ssize_t latency_timer_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *valbuf, size_t count);
> +static ssize_t event_char_store(struct device *dev,
> + struct device_attribute *attr, const char *valbuf, size_t count);
> +static ssize_t latency_timer_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +
Please work with the code so that you do not have to pre-define these
functions. It should be possible. Worst case, you pre-define the
structure for the driver, that should be it.
And again, have you tested this change?
thanks,
greg k-h
On Fri, Sep 02, 2022 at 05:56:13PM +0800, Greg KH wrote:
>> drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
>> 1 file changed, 51 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
>> index d5a3986dfee7..479c3a5caaf8 100644
>> --- a/drivers/usb/serial/ftdi_sio.c
>> +++ b/drivers/usb/serial/ftdi_sio.c
>> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
>> static u32 ftdi_232bm_baud_to_divisor(int baud);
>> static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
>> static u32 ftdi_2232h_baud_to_divisor(int baud);
>> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
>> + struct attribute *attr, int idx);
>> +static ssize_t latency_timer_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *valbuf, size_t count);
>> +static ssize_t event_char_store(struct device *dev,
>> + struct device_attribute *attr, const char *valbuf, size_t count);
>> +static ssize_t latency_timer_show(struct device *dev,
>> + struct device_attribute *attr, char *buf);
>> +
>
> Please work with the code so that you do not have to pre-define these
> functions. It should be possible. Worst case, you pre-define the
> structure for the driver, that should be it.
Without pre-definition of the functions, compilation errors will occur,
such as 'ftdi_sio_attr_is_visible' undeclared here.
I have no idea why they are not necessary.
Please explain in detail.
> And again, have you tested this change?
Every time I change the code, I recomplie it and check whether there are
errors.
Are there any other tests I need to do?
Thanks,
Jiang
On Fri, Sep 02, 2022 at 10:33:46PM +0800, Jiasheng Jiang wrote:
> On Fri, Sep 02, 2022 at 05:56:13PM +0800, Greg KH wrote:
> >> drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
> >> 1 file changed, 51 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> >> index d5a3986dfee7..479c3a5caaf8 100644
> >> --- a/drivers/usb/serial/ftdi_sio.c
> >> +++ b/drivers/usb/serial/ftdi_sio.c
> >> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
> >> static u32 ftdi_232bm_baud_to_divisor(int baud);
> >> static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
> >> static u32 ftdi_2232h_baud_to_divisor(int baud);
> >> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
> >> + struct attribute *attr, int idx);
> >> +static ssize_t latency_timer_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *valbuf, size_t count);
> >> +static ssize_t event_char_store(struct device *dev,
> >> + struct device_attribute *attr, const char *valbuf, size_t count);
> >> +static ssize_t latency_timer_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf);
> >> +
> >
> > Please work with the code so that you do not have to pre-define these
> > functions. It should be possible. Worst case, you pre-define the
> > structure for the driver, that should be it.
>
> Without pre-definition of the functions, compilation errors will occur,
> such as 'ftdi_sio_attr_is_visible' undeclared here.
> I have no idea why they are not necessary.
If you move the code around that asks for those functions, you will not
need to define them.
> > And again, have you tested this change?
>
> Every time I change the code, I recomplie it and check whether there are
> errors.
> Are there any other tests I need to do?
Yes, boot with the device and make sure that the sysfs files are still
there. You do have access to one of these devices, right? They are
very very common.
thanks,
greg k-h
On Fri, Sep 02, 2022 at 10:52:52PM +0800, Greg KH wrote:
>>>> drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
>>>> 1 file changed, 51 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
>>>> index d5a3986dfee7..479c3a5caaf8 100644
>>>> --- a/drivers/usb/serial/ftdi_sio.c
>>>> +++ b/drivers/usb/serial/ftdi_sio.c
>>>> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
>>>> static u32 ftdi_232bm_baud_to_divisor(int baud);
>>>> static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
>>>> static u32 ftdi_2232h_baud_to_divisor(int baud);
>>>> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
>>>> + struct attribute *attr, int idx);
>>>> +static ssize_t latency_timer_store(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *valbuf, size_t count);
>>>> +static ssize_t event_char_store(struct device *dev,
>>>> + struct device_attribute *attr, const char *valbuf, size_t count);
>>>> +static ssize_t latency_timer_show(struct device *dev,
>>>> + struct device_attribute *attr, char *buf);
>>>> +
>>>
>>> Please work with the code so that you do not have to pre-define these
>>> functions. It should be possible. Worst case, you pre-define the
>>> structure for the driver, that should be it.
>>
>> Without pre-definition of the functions, compilation errors will occur,
>> such as 'ftdi_sio_attr_is_visible' undeclared here.
>> I have no idea why they are not necessary.
>
> If you move the code around that asks for those functions, you will not
> need to define them.
>
Fine, I have already revised the patch and submitted a v4.
>>> And again, have you tested this change?
>>
>> Every time I change the code, I recomplie it and check whether there are
>> errors.
>> Are there any other tests I need to do?
>
> Yes, boot with the device and make sure that the sysfs files are still
> there. You do have access to one of these devices, right? They are
> very very common.
Sorry, I still have no idea how to boot with the device.
But if there is any wrong with the patch, you can tell me and I will continue
to revise it.
Thanks,
Jiang
On Mon, Sep 05, 2022 at 06:01:01PM +0800, Jiasheng Jiang wrote:
> On Fri, Sep 02, 2022 at 10:52:52PM +0800, Greg KH wrote:
> >>> And again, have you tested this change?
> >>
> >> Every time I change the code, I recomplie it and check whether there are
> >> errors.
> >> Are there any other tests I need to do?
> >
> > Yes, boot with the device and make sure that the sysfs files are still
> > there. You do have access to one of these devices, right? They are
> > very very common.
>
> Sorry, I still have no idea how to boot with the device.
You need to buy an FTDI device and plug it into a machine with your
patch applied to the running and make sure that you did not break
anything.
> But if there is any wrong with the patch, you can tell me and I will continue
> to revise it.
It is your job to test your patches.
Johan