Hi Luiz,
On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Hsin-chen,
>
> On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> >
> > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> >
> > Use device group to avoid the racing. To reuse the group defined in
> > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > read_usb_alt_setting which are only registered in btusb.
> >
> > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > Signed-off-by: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > ---
> >
> > (no changes since v1)
> >
> > drivers/bluetooth/btusb.c | 42 ++++++++------------------------
> > include/net/bluetooth/hci_core.h | 2 ++
> > net/bluetooth/hci_sysfs.c | 33 +++++++++++++++++++++++++
> > 3 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 75a0f15819c4..bf210275e5b7 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > return 0;
> > }
> >
> > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > +{
> > + struct btusb_data *data = hci_get_drvdata(hdev);
> > +
> > + return data->isoc_altsetting;
> > +}
> > +
> > static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> > int alt)
> > {
> > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> > .llseek = default_llseek,
> > };
> >
> > -static ssize_t isoc_alt_show(struct device *dev,
> > - struct device_attribute *attr,
> > - char *buf)
> > -{
> > - struct btusb_data *data = dev_get_drvdata(dev);
> > -
> > - return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > -}
> > -
> > -static ssize_t isoc_alt_store(struct device *dev,
> > - struct device_attribute *attr,
> > - const char *buf, size_t count)
> > -{
> > - struct btusb_data *data = dev_get_drvdata(dev);
> > - int alt;
> > - int ret;
> > -
> > - if (kstrtoint(buf, 10, &alt))
> > - return -EINVAL;
> > -
> > - ret = btusb_switch_alt_setting(data->hdev, alt);
> > - return ret < 0 ? ret : count;
> > -}
> > -
> > -static DEVICE_ATTR_RW(isoc_alt);
> > -
> > static int btusb_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > {
> > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> > if (err < 0)
> > goto out_free_dev;
> >
> > - err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > - if (err)
> > - goto out_free_dev;
> > + hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > + hdev->read_usb_alt_setting = btusb_read_alt_setting;
> > }
> >
> > if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > hdev = data->hdev;
> > usb_set_intfdata(data->intf, NULL);
> >
> > - if (data->isoc) {
> > - device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > + if (data->isoc)
> > usb_set_intfdata(data->isoc, NULL);
> > - }
> >
> > if (data->diag)
> > usb_set_intfdata(data->diag, NULL);
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f756fac95488..5d3ec5ff5adb 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -641,6 +641,8 @@ struct hci_dev {
> > struct bt_codec *codec, __u8 *vnd_len,
> > __u8 **vnd_data);
> > u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > + int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > + int (*read_usb_alt_setting)(struct hci_dev *hdev);
> > };
> >
> > #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 041ce9adc378..887aa1935b1e 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_WO(reset);
> >
> > +static ssize_t isoc_alt_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct hci_dev *hdev = to_hci_dev(dev);
> > +
> > + if (hdev->read_usb_alt_setting)
> > + return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > +
> > + return -ENODEV;
> > +}
> > +
> > +static ssize_t isoc_alt_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct hci_dev *hdev = to_hci_dev(dev);
> > + int alt;
> > + int ret;
> > +
> > + if (kstrtoint(buf, 10, &alt))
> > + return -EINVAL;
> > +
> > + if (hdev->switch_usb_alt_setting) {
> > + ret = hdev->switch_usb_alt_setting(hdev, alt);
> > + return ret < 0 ? ret : count;
> > + }
> > +
> > + return -ENODEV;
> > +}
> > +static DEVICE_ATTR_RW(isoc_alt);
> > +
> > static struct attribute *bt_host_attrs[] = {
> > &dev_attr_reset.attr,
> > + &dev_attr_isoc_alt.attr,
> > NULL,
> > };
> > ATTRIBUTE_GROUPS(bt_host);
>
> While this fixes the race it also forces the inclusion of an attribute
> that is driver specific, so I wonder if we should introduce some
> internal interface to register driver specific entries like this.
Do you mean you prefer the original interface that only exports the
attribute when isoc_altsetting is supported?
Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
called earlier than data->isoc is determined. I need some time to
verify whether changing the order won't break anything.
>
> > --
> > 2.48.1.262.g85cc9f2d1e-goog
> >
>
>
> --
> Luiz Augusto von Dentz
