Hi Hsin-chen,
On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> 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.
A quick git grep shows that this exists:
Documentation/ABI/testing/sysfs-bus-usb:What:
/sys/bus/usb/devices/usbX/bAlternateSetting
> > >
> > > > --
> > > > 2.48.1.262.g85cc9f2d1e-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
