| Message ID | 1478073426-3714-4-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
Sorry, I was at Plumbers and extra busy with other stuff. Just getting around to reviewing this. On 11/02/2016 02:57 AM, Cédric Le Goater wrote: > We could also use an ioctl for that purpose. sysfs seems a better > approach. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > drivers/char/ipmi/bt-bmc.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c > index e751e4a754b7..d7146f0e900e 100644 > --- a/drivers/char/ipmi/bt-bmc.c > +++ b/drivers/char/ipmi/bt-bmc.c > @@ -84,6 +84,33 @@ struct bt_bmc { > > static atomic_t open_count = ATOMIC_INIT(0); > > +static ssize_t bt_bmc_show_response_time(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); > + > + return snprintf(buf, PAGE_SIZE - 1, "%d\n", bt_bmc->response_time); > +} > + > +static ssize_t bt_bmc_set_response_time(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); > + unsigned long val; > + int err; > + > + err = kstrtoul(buf, 0, &val); > + if (err) > + return err; > + bt_bmc->response_time = val; > + return count; > +} > + > +static DEVICE_ATTR(response_time, 0644, > + bt_bmc_show_response_time, bt_bmc_set_response_time); > + > static u8 bt_inb(struct bt_bmc *bt_bmc, int reg) > { > return ioread8(bt_bmc->base + reg); > @@ -572,6 +599,10 @@ static int bt_bmc_probe(struct platform_device *pdev) > bt_bmc_config_irq(bt_bmc, pdev); > > bt_bmc->response_time = BT_BMC_RESPONSE_TIME; > + rc = device_create_file(&pdev->dev, &dev_attr_response_time); > + if (rc) > + dev_warn(&pdev->dev, "can't create response_time file\n"); > + You added an extra space here. > > if (bt_bmc->irq) { > dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
On 11/07/2016 07:37 PM, Corey Minyard wrote: > Sorry, I was at Plumbers and extra busy with other stuff. Just getting around to reviewing this. > > On 11/02/2016 02:57 AM, Cédric Le Goater wrote: >> We could also use an ioctl for that purpose. sysfs seems a better >> approach. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> drivers/char/ipmi/bt-bmc.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c >> index e751e4a754b7..d7146f0e900e 100644 >> --- a/drivers/char/ipmi/bt-bmc.c >> +++ b/drivers/char/ipmi/bt-bmc.c >> @@ -84,6 +84,33 @@ struct bt_bmc { >> static atomic_t open_count = ATOMIC_INIT(0); >> +static ssize_t bt_bmc_show_response_time(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); >> + >> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", bt_bmc->response_time); >> +} >> + >> +static ssize_t bt_bmc_set_response_time(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); >> + unsigned long val; >> + int err; >> + >> + err = kstrtoul(buf, 0, &val); >> + if (err) >> + return err; >> + bt_bmc->response_time = val; >> + return count; >> +} >> + >> +static DEVICE_ATTR(response_time, 0644, >> + bt_bmc_show_response_time, bt_bmc_set_response_time); >> + >> static u8 bt_inb(struct bt_bmc *bt_bmc, int reg) >> { >> return ioread8(bt_bmc->base + reg); >> @@ -572,6 +599,10 @@ static int bt_bmc_probe(struct platform_device *pdev) >> bt_bmc_config_irq(bt_bmc, pdev); >> bt_bmc->response_time = BT_BMC_RESPONSE_TIME; >> + rc = device_create_file(&pdev->dev, &dev_attr_response_time); >> + if (rc) >> + dev_warn(&pdev->dev, "can't create response_time file\n"); >> + > > You added an extra space here. yes. I will remove it in the next version. The patch raises a few questions on the "BT Interface Capabilities" : - should we use sysfs or a specific ioctl to configure the driver ? - should the driver handle directly the response to the "Get BT Interface Capabilities" command ? What is your opinion ? Thanks, C. >> if (bt_bmc->irq) { >> dev_info(dev, "Using IRQ %d\n", bt_bmc->irq); > >
On 11/09/2016 08:42 AM, Cédric Le Goater wrote: > On 11/07/2016 07:37 PM, Corey Minyard wrote: >> Sorry, I was at Plumbers and extra busy with other stuff. Just getting around to reviewing this. >> >> On 11/02/2016 02:57 AM, Cédric Le Goater wrote: >>> We could also use an ioctl for that purpose. sysfs seems a better >>> approach. >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> drivers/char/ipmi/bt-bmc.c | 31 +++++++++++++++++++++++++++++++ >>> 1 file changed, 31 insertions(+) >>> >>> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c >>> index e751e4a754b7..d7146f0e900e 100644 >>> --- a/drivers/char/ipmi/bt-bmc.c >>> +++ b/drivers/char/ipmi/bt-bmc.c >>> @@ -84,6 +84,33 @@ struct bt_bmc { >>> static atomic_t open_count = ATOMIC_INIT(0); >>> +static ssize_t bt_bmc_show_response_time(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); >>> + >>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", bt_bmc->response_time); >>> +} >>> + >>> +static ssize_t bt_bmc_set_response_time(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); >>> + unsigned long val; >>> + int err; >>> + >>> + err = kstrtoul(buf, 0, &val); >>> + if (err) >>> + return err; >>> + bt_bmc->response_time = val; >>> + return count; >>> +} >>> + >>> +static DEVICE_ATTR(response_time, 0644, >>> + bt_bmc_show_response_time, bt_bmc_set_response_time); >>> + >>> static u8 bt_inb(struct bt_bmc *bt_bmc, int reg) >>> { >>> return ioread8(bt_bmc->base + reg); >>> @@ -572,6 +599,10 @@ static int bt_bmc_probe(struct platform_device *pdev) >>> bt_bmc_config_irq(bt_bmc, pdev); >>> bt_bmc->response_time = BT_BMC_RESPONSE_TIME; >>> + rc = device_create_file(&pdev->dev, &dev_attr_response_time); >>> + if (rc) >>> + dev_warn(&pdev->dev, "can't create response_time file\n"); >>> + >> You added an extra space here. > yes. I will remove it in the next version. > > The patch raises a few questions on the "BT Interface Capabilities" : > > - should we use sysfs or a specific ioctl to configure the driver ? I should probably say sysfs, but really I don't care. The hard part about ioctls is the compat, and there shouldn't be any compat issues with this interface. An ioctl is probably easier, especially if you add an ioctl for the header size thing I talked about in the previous email. The only thing that matters to the driver is the timeout. If you want to make the timeout per fd, then it will have to be an ioctl. > - should the driver handle directly the response to the "Get BT > Interface Capabilities" command ? That probably belongs in userspace. The only reason I can think of to have it in the driver would be so the host driver can fetch it if the BMC userspace is down, but I can't see how that's a good idea. Hope my wishy-washy answer helps :-). -corey > What is your opinion ? > > Thanks, > > C. > >>> if (bt_bmc->irq) { >>> dev_info(dev, "Using IRQ %d\n", bt_bmc->irq); >>
On Thu, Nov 10, 2016 at 2:34 AM, Corey Minyard <minyard@acm.org> wrote: > On 11/09/2016 08:42 AM, Cédric Le Goater wrote: >> The patch raises a few questions on the "BT Interface Capabilities" : >> >> - should we use sysfs or a specific ioctl to configure the driver ? > > > I should probably say sysfs, but really I don't care. The hard part about > ioctls is the compat, and there shouldn't be any compat issues with this > interface. An ioctl is probably easier, especially if you add an ioctl for > the header size thing I talked about in the previous email. > > The only thing that matters to the driver is the timeout. If you want to > make the timeout per fd, then it will have to be an ioctl. I vote for an ioctl as it's simpler for userspace. In another driver we use on the BMCs we have a character device and a few sysfs files for configuration. This means userspace needs to discover and open > 1 fd, which is annoying. Cheers, Joel >> - should the driver handle directly the response to the "Get BT >> Interface Capabilities" command ? > > > That probably belongs in userspace. The only reason I can think of > to have it in the driver would be so the host driver can fetch it if the > BMC userspace is down, but I can't see how that's a good idea. > > Hope my wishy-washy answer helps :-).
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c index e751e4a754b7..d7146f0e900e 100644 --- a/drivers/char/ipmi/bt-bmc.c +++ b/drivers/char/ipmi/bt-bmc.c @@ -84,6 +84,33 @@ struct bt_bmc { static atomic_t open_count = ATOMIC_INIT(0); +static ssize_t bt_bmc_show_response_time(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); + + return snprintf(buf, PAGE_SIZE - 1, "%d\n", bt_bmc->response_time); +} + +static ssize_t bt_bmc_set_response_time(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct bt_bmc *bt_bmc = dev_get_drvdata(dev); + unsigned long val; + int err; + + err = kstrtoul(buf, 0, &val); + if (err) + return err; + bt_bmc->response_time = val; + return count; +} + +static DEVICE_ATTR(response_time, 0644, + bt_bmc_show_response_time, bt_bmc_set_response_time); + static u8 bt_inb(struct bt_bmc *bt_bmc, int reg) { return ioread8(bt_bmc->base + reg); @@ -572,6 +599,10 @@ static int bt_bmc_probe(struct platform_device *pdev) bt_bmc_config_irq(bt_bmc, pdev); bt_bmc->response_time = BT_BMC_RESPONSE_TIME; + rc = device_create_file(&pdev->dev, &dev_attr_response_time); + if (rc) + dev_warn(&pdev->dev, "can't create response_time file\n"); + if (bt_bmc->irq) { dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
We could also use an ioctl for that purpose. sysfs seems a better approach. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- drivers/char/ipmi/bt-bmc.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)