| Message ID | 20210928213653.3028439-2-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | [v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
Hi Luiz, > This adds force_suspend which can be used to force the controller into > suspend/resume state. > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > index cc3679f3491d..52f9106faeae 100644 > --- a/drivers/bluetooth/hci_vhci.c > +++ b/drivers/bluetooth/hci_vhci.c > @@ -21,6 +21,7 @@ > > #include <linux/skbuff.h> > #include <linux/miscdevice.h> > +#include <linux/debugfs.h> > > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type, > return 0; > } > > +static ssize_t force_suspend_read(struct file *file, char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct hci_dev *hdev = file->private_data; > + char buf[3]; > + > + buf[0] = hdev->suspended ? 'Y' : 'N'; > + buf[1] = '\n'; > + buf[2] = '\0'; > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); > +} > + > +static ssize_t force_suspend_write(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct hci_dev *hdev = file->private_data; > + bool enable; > + int err; > + > + err = kstrtobool_from_user(user_buf, count, &enable); > + if (err) > + return err; > + > + if (hdev->suspended == enable) > + return -EALREADY; I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver. > + > + if (enable) > + err = hci_suspend_dev(hdev); > + else > + err = hci_resume_dev(hdev); > + > + if (err) > + return err; > + > + return count; > +} > + > +static const struct file_operations force_suspend_fops = { > + .open = simple_open, > + .read = force_suspend_read, > + .write = force_suspend_write, > + .llseek = default_llseek, > +}; > + > static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > { > struct hci_dev *hdev; > @@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > return -EBUSY; > } > > + debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev, > + &force_suspend_fops); > + > hci_skb_pkt_type(skb) = HCI_VENDOR_PKT; Regards Marcel
Hi Marcel, On Wed, Sep 29, 2021 at 6:51 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > This adds force_suspend which can be used to force the controller into > > suspend/resume state. > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > > index cc3679f3491d..52f9106faeae 100644 > > --- a/drivers/bluetooth/hci_vhci.c > > +++ b/drivers/bluetooth/hci_vhci.c > > @@ -21,6 +21,7 @@ > > > > #include <linux/skbuff.h> > > #include <linux/miscdevice.h> > > +#include <linux/debugfs.h> > > > > #include <net/bluetooth/bluetooth.h> > > #include <net/bluetooth/hci_core.h> > > @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type, > > return 0; > > } > > > > +static ssize_t force_suspend_read(struct file *file, char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct hci_dev *hdev = file->private_data; > > + char buf[3]; > > + > > + buf[0] = hdev->suspended ? 'Y' : 'N'; > > + buf[1] = '\n'; > > + buf[2] = '\0'; > > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); > > +} > > + > > +static ssize_t force_suspend_write(struct file *file, > > + const char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct hci_dev *hdev = file->private_data; > > + bool enable; > > + int err; > > + > > + err = kstrtobool_from_user(user_buf, count, &enable); > > + if (err) > > + return err; > > + > > + if (hdev->suspended == enable) > > + return -EALREADY; > > I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver. Ok, but I guess hdev->suspended should be kept as is, right? > > > + > > + if (enable) > > + err = hci_suspend_dev(hdev); > > + else > > + err = hci_resume_dev(hdev); > > + > > + if (err) > > + return err; > > + > > + return count; > > +} > > + > > +static const struct file_operations force_suspend_fops = { > > + .open = simple_open, > > + .read = force_suspend_read, > > + .write = force_suspend_write, > > + .llseek = default_llseek, > > +}; > > + > > static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > { > > struct hci_dev *hdev; > > @@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > return -EBUSY; > > } > > > > + debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev, > > + &force_suspend_fops); > > + > > hci_skb_pkt_type(skb) = HCI_VENDOR_PKT; > > Regards > > Marcel >
Hi Luiz, >>> This adds force_suspend which can be used to force the controller into >>> suspend/resume state. >>> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>> --- >>> drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 49 insertions(+) >>> >>> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c >>> index cc3679f3491d..52f9106faeae 100644 >>> --- a/drivers/bluetooth/hci_vhci.c >>> +++ b/drivers/bluetooth/hci_vhci.c >>> @@ -21,6 +21,7 @@ >>> >>> #include <linux/skbuff.h> >>> #include <linux/miscdevice.h> >>> +#include <linux/debugfs.h> >>> >>> #include <net/bluetooth/bluetooth.h> >>> #include <net/bluetooth/hci_core.h> >>> @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type, >>> return 0; >>> } >>> >>> +static ssize_t force_suspend_read(struct file *file, char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct hci_dev *hdev = file->private_data; >>> + char buf[3]; >>> + >>> + buf[0] = hdev->suspended ? 'Y' : 'N'; >>> + buf[1] = '\n'; >>> + buf[2] = '\0'; >>> + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); >>> +} >>> + >>> +static ssize_t force_suspend_write(struct file *file, >>> + const char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct hci_dev *hdev = file->private_data; >>> + bool enable; >>> + int err; >>> + >>> + err = kstrtobool_from_user(user_buf, count, &enable); >>> + if (err) >>> + return err; >>> + >>> + if (hdev->suspended == enable) >>> + return -EALREADY; >> >> I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver. > > Ok, but I guess hdev->suspended should be kept as is, right? yes, that is for the core to handle. The transport driver just needs to not mess or depend on core internals. Regards Marcel
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c index cc3679f3491d..52f9106faeae 100644 --- a/drivers/bluetooth/hci_vhci.c +++ b/drivers/bluetooth/hci_vhci.c @@ -21,6 +21,7 @@ #include <linux/skbuff.h> #include <linux/miscdevice.h> +#include <linux/debugfs.h> #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type, return 0; } +static ssize_t force_suspend_read(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct hci_dev *hdev = file->private_data; + char buf[3]; + + buf[0] = hdev->suspended ? 'Y' : 'N'; + buf[1] = '\n'; + buf[2] = '\0'; + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); +} + +static ssize_t force_suspend_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct hci_dev *hdev = file->private_data; + bool enable; + int err; + + err = kstrtobool_from_user(user_buf, count, &enable); + if (err) + return err; + + if (hdev->suspended == enable) + return -EALREADY; + + if (enable) + err = hci_suspend_dev(hdev); + else + err = hci_resume_dev(hdev); + + if (err) + return err; + + return count; +} + +static const struct file_operations force_suspend_fops = { + .open = simple_open, + .read = force_suspend_read, + .write = force_suspend_write, + .llseek = default_llseek, +}; + static int __vhci_create_device(struct vhci_data *data, __u8 opcode) { struct hci_dev *hdev; @@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) return -EBUSY; } + debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev, + &force_suspend_fops); + hci_skb_pkt_type(skb) = HCI_VENDOR_PKT; skb_put_u8(skb, 0xff);