On Sat, Jul 10, 2010 at 12:14:45PM -0500, Narendra K wrote:
> +static char smbios_attr[4096];
Interesting size, too bad you don't reference it again:
> + if (attribute == SMBIOS_ATTR_INSTANCE_SHOW)
> + return scnprintf(smbios_attr, PAGE_SIZE,
> + "%d\n", donboard->instance);
> + else if (attribute == SMBIOS_ATTR_LABEL_SHOW)
> + return scnprintf(smbios_attr, PAGE_SIZE,
> + "%s\n", dmi->name);
PAGE_SIZE here?
Which is it (note, some arches PAGE_SIZE is not 4k...)
> + return strlen(dmi->name);
> + }
> + }
> + return 0;
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + ssize_t result;
> + result = smbios_instance_string_exist(&dev->kobj, &attr->attr,
> + SMBIOS_ATTR_LABEL_SHOW);
> + strncpy(buf, smbios_attr, result);
> + return result;
> +}
> +
> +static ssize_t
> +smbiosinstance_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + ssize_t result;
> + result = smbios_instance_string_exist(&dev->kobj, &attr->attr,
> + SMBIOS_ATTR_INSTANCE_SHOW);
> + strncpy(buf, smbios_attr, result);
> + return result;
> +}
> +
> +static struct device_attribute smbios_attr_label = {
> + .attr = {.name = "label", .mode = 0444, .owner = THIS_MODULE},
> + .show = smbiosname_show,
> +};
> +
> +static struct device_attribute smbios_attr_instance = {
> + .attr = {.name = "index", .mode = 0444, .owner = THIS_MODULE},
> + .show = smbiosinstance_show,
> +};
> +
> +static struct attribute *smbios_attributes[] = {
> + &smbios_attr_label.attr,
> + &smbios_attr_instance.attr,
> + NULL,
> +};
> +
> +static struct attribute_group smbios_attr_group = {
> + .attrs = smbios_attributes,
> + .is_visible = smbios_instance_string_exist,
> +};
> +
> +static int
> +pci_create_smbiosname_file(struct pci_dev *pdev)
> +{
> + if (smbios_attr_group.is_visible &&
You set .is_visable above, how could it not be set?
> + smbios_attr_group.is_visible(&pdev->dev.kobj, NULL, 0)) {
> + if (!sysfs_create_group(&pdev->dev.kobj, &smbios_attr_group))
> + return 0;
No, sysfs_create_group will call .is_visable, right? You shouldn't need
to call it yourself or check it, at all.
> +static int
> +pci_remove_smbiosname_file(struct pci_dev *pdev)
> +{
> + if (smbios_attr_group.is_visible &&
> + smbios_attr_group.is_visible(&pdev->dev.kobj, NULL, 0)) {
> + sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
> + return 0;
Same here, you shouldn't have to check .is_visable.
> + }
> + return -1;
Why -1? Please use a real error value.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
