Hi Hsin-chen,
On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
>
> 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.
We might have to do something like the following within hci_init_sysfs:
if (hdev->isoc_alt)
dev->type = bt_host_isoc_alt
else
dev->type = bt_host
Now perhaps instead of adding the callbacks to hdev we add the
attribute itself, btw did you check if there isn't a sysfs entry to
interact with USB alternate settings? Because contrary to reset this
actually operates directly on the driver bus so it sort of made more
sense to me that this would be handled by USB rather than Bluetooth.
> >
> > > --
> > > 2.48.1.262.g85cc9f2d1e-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
