| Message ID | 20220707151420.v3.2.I39885624992dacff236aed268bdaa69107cd1310@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] Bluetooth: Add support for devcoredump | expand |
Hi Manish,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth/master]
[also build test WARNING on bluetooth-next/master linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20220708/202207081448.MghkbBvn-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/0d785cbd11ed3a6de29aeb05926177440ab26d54
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
git checkout 0d785cbd11ed3a6de29aeb05926177440ab26d54
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> net/bluetooth/hci_sysfs.c:118:1: sparse: sparse: symbol 'dev_attr_enable_coredump' was not declared. Should it be static?
Hi Manish, On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.com> wrote: > > Since device/firmware dump is a debugging feature, we may not need it > all the time. Add a sysfs attribute to enable/disable bluetooth > devcoredump capturing. The default state of this feature would be > disabled and it can be enabled by echoing 1 to enable_coredump sysfs > entry as follow: > > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump > > Signed-off-by: Manish Mandlik <mmandlik@google.com> > --- > > Changes in v3: > - New patch in the series to enable/disable feature via sysfs entry > > net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c > index 4e3e0451b08c..df0d54a5ae3f 100644 > --- a/net/bluetooth/hci_sysfs.c > +++ b/net/bluetooth/hci_sysfs.c > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev) > module_put(THIS_MODULE); > } > > +#ifdef CONFIG_DEV_COREDUMP > +static ssize_t enable_coredump_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct hci_dev *hdev = to_hci_dev(dev); > + > + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled); > +} > + > +static ssize_t enable_coredump_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hci_dev *hdev = to_hci_dev(dev); > + > + /* Consider any non-zero value as true */ > + if (simple_strtol(buf, NULL, 10)) > + hdev->dump.enabled = true; > + else > + hdev->dump.enabled = false; > + > + return count; > +} > +DEVICE_ATTR_RW(enable_coredump); > +#endif > + > +static struct attribute *bt_host_attrs[] = { > +#ifdef CONFIG_DEV_COREDUMP > + &dev_attr_enable_coredump.attr, > +#endif > + NULL, > +}; > +ATTRIBUTE_GROUPS(bt_host); > + > static const struct device_type bt_host = { > .name = "host", > .release = bt_host_release, > + .groups = bt_host_groups, > }; > > void hci_init_sysfs(struct hci_dev *hdev) > -- > 2.37.0.rc0.161.g10f37bed90-goog It seems devcoredump.c creates its own sysfs entries so perhaps this should be included there and documented in sysfs-devices-coredump.
Hi Manish, On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@google.com> wrote: > > Hi Luiz, > > On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> >> Hi Manish, >> >> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.com> wrote: >> > >> > Since device/firmware dump is a debugging feature, we may not need it >> > all the time. Add a sysfs attribute to enable/disable bluetooth >> > devcoredump capturing. The default state of this feature would be >> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs >> > entry as follow: >> > >> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump >> > >> > Signed-off-by: Manish Mandlik <mmandlik@google.com> >> > --- >> > >> > Changes in v3: >> > - New patch in the series to enable/disable feature via sysfs entry >> > >> > net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 36 insertions(+) >> > >> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c >> > index 4e3e0451b08c..df0d54a5ae3f 100644 >> > --- a/net/bluetooth/hci_sysfs.c >> > +++ b/net/bluetooth/hci_sysfs.c >> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev) >> > module_put(THIS_MODULE); >> > } >> > >> > +#ifdef CONFIG_DEV_COREDUMP >> > +static ssize_t enable_coredump_show(struct device *dev, >> > + struct device_attribute *attr, >> > + char *buf) >> > +{ >> > + struct hci_dev *hdev = to_hci_dev(dev); >> > + >> > + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled); >> > +} >> > + >> > +static ssize_t enable_coredump_store(struct device *dev, >> > + struct device_attribute *attr, >> > + const char *buf, size_t count) >> > +{ >> > + struct hci_dev *hdev = to_hci_dev(dev); >> > + >> > + /* Consider any non-zero value as true */ >> > + if (simple_strtol(buf, NULL, 10)) >> > + hdev->dump.enabled = true; >> > + else >> > + hdev->dump.enabled = false; >> > + >> > + return count; >> > +} >> > +DEVICE_ATTR_RW(enable_coredump); >> > +#endif >> > + >> > +static struct attribute *bt_host_attrs[] = { >> > +#ifdef CONFIG_DEV_COREDUMP >> > + &dev_attr_enable_coredump.attr, >> > +#endif >> > + NULL, >> > +}; >> > +ATTRIBUTE_GROUPS(bt_host); >> > + >> > static const struct device_type bt_host = { >> > .name = "host", >> > .release = bt_host_release, >> > + .groups = bt_host_groups, >> > }; >> > >> > void hci_init_sysfs(struct hci_dev *hdev) >> > -- >> > 2.37.0.rc0.161.g10f37bed90-goog >> >> It seems devcoredump.c creates its own sysfs entries so perhaps this >> should be included there and documented in sysfs-devices-coredump. > > I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump. We should probably check if there is a reason why this is not on per device and anyway if seem wrong to each subsystem to come up with its own sysfs entries when it could be easily generalized so the userspace tools using those entries don't have to look into different entries depending on the subsystem the device belongs. > >> >> -- >> Luiz Augusto von Dentz > > > Regards, > Manish.
Hi Manish, On Fri, Jul 8, 2022 at 3:30 PM Manish Mandlik <mmandlik@google.com> wrote: > > Hi Luiz, > > On Fri, Jul 8, 2022 at 11:40 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> >> Hi Manish, >> >> On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@google.com> wrote: >> > >> > Hi Luiz, >> > >> > On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> >> >> >> Hi Manish, >> >> >> >> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.com> wrote: >> >> > >> >> > Since device/firmware dump is a debugging feature, we may not need it >> >> > all the time. Add a sysfs attribute to enable/disable bluetooth >> >> > devcoredump capturing. The default state of this feature would be >> >> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs >> >> > entry as follow: >> >> > >> >> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump >> >> > >> >> > Signed-off-by: Manish Mandlik <mmandlik@google.com> >> >> > --- >> >> > >> >> > Changes in v3: >> >> > - New patch in the series to enable/disable feature via sysfs entry >> >> > >> >> > net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++ >> >> > 1 file changed, 36 insertions(+) >> >> > >> >> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c >> >> > index 4e3e0451b08c..df0d54a5ae3f 100644 >> >> > --- a/net/bluetooth/hci_sysfs.c >> >> > +++ b/net/bluetooth/hci_sysfs.c >> >> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev) >> >> > module_put(THIS_MODULE); >> >> > } >> >> > >> >> > +#ifdef CONFIG_DEV_COREDUMP >> >> > +static ssize_t enable_coredump_show(struct device *dev, >> >> > + struct device_attribute *attr, >> >> > + char *buf) >> >> > +{ >> >> > + struct hci_dev *hdev = to_hci_dev(dev); >> >> > + >> >> > + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled); >> >> > +} >> >> > + >> >> > +static ssize_t enable_coredump_store(struct device *dev, >> >> > + struct device_attribute *attr, >> >> > + const char *buf, size_t count) >> >> > +{ >> >> > + struct hci_dev *hdev = to_hci_dev(dev); >> >> > + >> >> > + /* Consider any non-zero value as true */ >> >> > + if (simple_strtol(buf, NULL, 10)) >> >> > + hdev->dump.enabled = true; >> >> > + else >> >> > + hdev->dump.enabled = false; >> >> > + >> >> > + return count; >> >> > +} >> >> > +DEVICE_ATTR_RW(enable_coredump); >> >> > +#endif >> >> > + >> >> > +static struct attribute *bt_host_attrs[] = { >> >> > +#ifdef CONFIG_DEV_COREDUMP >> >> > + &dev_attr_enable_coredump.attr, >> >> > +#endif >> >> > + NULL, >> >> > +}; >> >> > +ATTRIBUTE_GROUPS(bt_host); >> >> > + >> >> > static const struct device_type bt_host = { >> >> > .name = "host", >> >> > .release = bt_host_release, >> >> > + .groups = bt_host_groups, >> >> > }; >> >> > >> >> > void hci_init_sysfs(struct hci_dev *hdev) >> >> > -- >> >> > 2.37.0.rc0.161.g10f37bed90-goog >> >> >> >> It seems devcoredump.c creates its own sysfs entries so perhaps this >> >> should be included there and documented in sysfs-devices-coredump. >> > >> > I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump. >> >> We should probably check if there is a reason why this is not on per >> device and anyway if seem wrong to each subsystem to come up with its >> own sysfs entries when it could be easily generalized so the userspace >> tools using those entries don't have to look into different entries >> depending on the subsystem the device belongs. > > The purpose of the base devcoredump interface is to only provide a generalized mechanism for drivers to dump the firmware data. It is not aware of which subsystem is using it or what data is being dumped. We want to implement something on top of it only for all bluetooth drivers so that they all can generate firmware dumps in a common way and not worry about the synchronizations and timeouts. That's why it made more sense to me to have a bluetooth specific sysfs entry for enabling/disabling only the bluetooth devcoredump interface without affecting the other users of the base devcoredump. It looks like we are not understanding each other, you are saying devcoredump only provides a generalized mechanism for the driver to dump the firmware coredump, yet we are implementing something extra to generalize it for bluetooth drivers? Making it bluetooth specific sort of defeat the purpose of a common layer in my opinion and it is not like the devcoredump.c don't already have entries inside the device sysfs directory as documented in sysfs-devices-coredump: What: /sys/devices/.../coredump Date: December 2017 Contact: Arend van Spriel <aspriel@gmail.com> Description: The /sys/devices/.../coredump attribute is only present when the device is bound to a driver, which provides the .coredump() callback. The attribute is write only. Anything written to this file will trigger the .coredump() callback. Available when CONFIG_DEV_COREDUMP is enabled. What I'm suggesting is in addition to /sys/devices/.../coredump we also have /sys/devices/.../enable_coredump, perhaps we should include in the discussion since he is listed as maintainer of DEV_COREDUMP nowadays. > Do you suggest we have this sysfs entry for bluetooth class instead? like "/sys/class/bluetooth/enable_coredump" instead of "/sys/class/bluetooth/<device>/enable_coredump"? But the problem with this is that if we have more than one bluetooth controller on the system, disabling one would disable the other as well. So to have the flexibility of controlling it for each device independently I am creating a sysfs entry for each device. IMO, the base devcoredump class sysfs entry is not much of a use anymore as there are already other systems like wifi using it. Let me know your thoughts. > >> >> >> > >> >> >> >> -- >> >> Luiz Augusto von Dentz >> > >> > >> > Regards, >> > Manish. >> >> >> >> -- >> Luiz Augusto von Dentz > > > Regards, > Manish.
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c index 4e3e0451b08c..df0d54a5ae3f 100644 --- a/net/bluetooth/hci_sysfs.c +++ b/net/bluetooth/hci_sysfs.c @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev) module_put(THIS_MODULE); } +#ifdef CONFIG_DEV_COREDUMP +static ssize_t enable_coredump_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct hci_dev *hdev = to_hci_dev(dev); + + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled); +} + +static ssize_t enable_coredump_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct hci_dev *hdev = to_hci_dev(dev); + + /* Consider any non-zero value as true */ + if (simple_strtol(buf, NULL, 10)) + hdev->dump.enabled = true; + else + hdev->dump.enabled = false; + + return count; +} +DEVICE_ATTR_RW(enable_coredump); +#endif + +static struct attribute *bt_host_attrs[] = { +#ifdef CONFIG_DEV_COREDUMP + &dev_attr_enable_coredump.attr, +#endif + NULL, +}; +ATTRIBUTE_GROUPS(bt_host); + static const struct device_type bt_host = { .name = "host", .release = bt_host_release, + .groups = bt_host_groups, }; void hci_init_sysfs(struct hci_dev *hdev)
Since device/firmware dump is a debugging feature, we may not need it all the time. Add a sysfs attribute to enable/disable bluetooth devcoredump capturing. The default state of this feature would be disabled and it can be enabled by echoing 1 to enable_coredump sysfs entry as follow: $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump Signed-off-by: Manish Mandlik <mmandlik@google.com> --- Changes in v3: - New patch in the series to enable/disable feature via sysfs entry net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)