Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Pavel, Am Dienstag, den 12.11.2013, 14:50 +0100 schrieb Pavel Machek: > Hi! > > > The GenWQE device is a PCIe card used to acclerate different tasks. > > Since it is configurable, it can be adjusted to different purposes. > > Our initial task for the card is to do zlib style compression/decompression > > RFC1950, RFC1951, and RFC1952. > > Is it similar to the Intel's Xeon Phi? They basically many-core have > computer on PCI card... > No, I think it is not the same. The GenWQE PCIe card is not a multi-processor card. One cannot run software on it. Our GenWQE card can be configured by updating it with different bitstreams to perform different purposes. The service logic to perform such updates stays of course the same and the work-queue too. But the request one can put onto this work-queue can be of different nature. Currently we can do things like compression, decompression, calculate a CRC, copy data ... Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Pavel, Am Dienstag, den 12.11.2013, 14:50 +0100 schrieb Pavel Machek: Hi! The GenWQE device is a PCIe card used to acclerate different tasks. Since it is configurable, it can be adjusted to different purposes. Our initial task for the card is to do zlib style compression/decompression RFC1950, RFC1951, and RFC1952. Is it similar to the Intel's Xeon Phi? They basically many-core have computer on PCI card... No, I think it is not the same. The GenWQE PCIe card is not a multi-processor card. One cannot run software on it. Our GenWQE card can be configured by updating it with different bitstreams to perform different purposes. The service logic to perform such updates stays of course the same and the work-queue too. But the request one can put onto this work-queue can be of different nature. Currently we can do things like compression, decompression, calculate a CRC, copy data ... Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi! > The GenWQE device is a PCIe card used to acclerate different tasks. > Since it is configurable, it can be adjusted to different purposes. > Our initial task for the card is to do zlib style compression/decompression > RFC1950, RFC1951, and RFC1952. Is it similar to the Intel's Xeon Phi? They basically many-core have computer on PCI card... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi! The GenWQE device is a PCIe card used to acclerate different tasks. Since it is configurable, it can be adjusted to different purposes. Our initial task for the card is to do zlib style compression/decompression RFC1950, RFC1951, and RFC1952. Is it similar to the Intel's Xeon Phi? They basically many-core have computer on PCI card... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Tue, Nov 05, 2013 at 08:16:24AM +0100, Frank Haverkamp wrote: > Hi Greg, > > Am Montag, den 04.11.2013, 14:15 -0800 schrieb Greg KH: > > > I am using sysfs_create_group() now, but do I understand you > > correctly > > > that setting the const struct attribute_group **groups; in my device > > > (where in my struct pci_device.dev?) is an even better way to > > establish > > > my sysfs attributes? Is there a function which I could call rather > > than > > > trying to find the right pointer? > > > > Ugh, this is still a problem, I'm trying to work through how to have > > individual drivers implement groups and sysfs files in a non-racy way. > > The issue is your device was announced to userspace _before_ it was > > bound to the driver, so there's no way to get the sysfs files to apply > > before then. > > > > You should just have the attributes on the sysfs device you create > > yourself, not your pci device, as that's where they make more sense, > > and > > there you should be able to use the attribute group easily, right? > > > > The current version works well for me and I did not get any complaints > about the possible race-condition you mentioned so far. So I am happy. Just because you can't see the race happening, doesn't mean it isn't there and will not show up for someone else. A trivial way to see this is write a program that uses libudev that gets notified when your device shows up and then read the attribute files. Those files will not be present when your program is notified so the read will usually fail. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Tue, Nov 05, 2013 at 08:16:24AM +0100, Frank Haverkamp wrote: Hi Greg, Am Montag, den 04.11.2013, 14:15 -0800 schrieb Greg KH: I am using sysfs_create_group() now, but do I understand you correctly that setting the const struct attribute_group **groups; in my device (where in my struct pci_device.dev?) is an even better way to establish my sysfs attributes? Is there a function which I could call rather than trying to find the right pointer? Ugh, this is still a problem, I'm trying to work through how to have individual drivers implement groups and sysfs files in a non-racy way. The issue is your device was announced to userspace _before_ it was bound to the driver, so there's no way to get the sysfs files to apply before then. You should just have the attributes on the sysfs device you create yourself, not your pci device, as that's where they make more sense, and there you should be able to use the attribute group easily, right? The current version works well for me and I did not get any complaints about the possible race-condition you mentioned so far. So I am happy. Just because you can't see the race happening, doesn't mean it isn't there and will not show up for someone else. A trivial way to see this is write a program that uses libudev that gets notified when your device shows up and then read the attribute files. Those files will not be present when your program is notified so the read will usually fail. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Montag, den 04.11.2013, 14:15 -0800 schrieb Greg KH: > > I am using sysfs_create_group() now, but do I understand you > correctly > > that setting the const struct attribute_group **groups; in my device > > (where in my struct pci_device.dev?) is an even better way to > establish > > my sysfs attributes? Is there a function which I could call rather > than > > trying to find the right pointer? > > Ugh, this is still a problem, I'm trying to work through how to have > individual drivers implement groups and sysfs files in a non-racy way. > The issue is your device was announced to userspace _before_ it was > bound to the driver, so there's no way to get the sysfs files to apply > before then. > > You should just have the attributes on the sysfs device you create > yourself, not your pci device, as that's where they make more sense, > and > there you should be able to use the attribute group easily, right? > The current version works well for me and I did not get any complaints about the possible race-condition you mentioned so far. So I am happy. > thanks, > > greg k-h Greetings Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Mon, Nov 04, 2013 at 05:41:27PM +0100, Frank Haverkamp wrote:
> Hi Greg,
>
> Am Mittwoch, den 30.10.2013, 10:44 -0700 schrieb Greg KH:
> > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
> > > +/*
> > > + * Create device_attribute structures / params: name, mode, show, store
> > > + * additional flag if valid in VF
> > > + */
> > > +struct genwqe_dev_attrib {
> > > + struct device_attribute att;/* sysfs entry attributes */
> > > + int vf; /* may exist in VF or not */
> > > +};
> >
> > Why do you need your own structure? Use the is_visible() callback to
> > create or not, the individual attributes for a specific device, don't
> > roll your own logic for something the driver core already supports.
> >
> > > +static struct genwqe_dev_attrib dev_attr_tab[] = {
> > > + {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
> > > + {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
> > > + show_card_next_bitstream, store_card_next_bitstream), 0},
> > > + {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
> > > + {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
> > > + {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
> > > +
> > > + {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
> > > + {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
> > > + {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
> > > + {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
> > > + {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
> > > +};
> > > +
> > > +/**
> > > + * create_card_sysfs() - Setup sysfs entries of the card device
> > > + *
> > > + * VFs have restricted mmio capabilities, so not all sysfs entries
> > > + * are allowed in VFs.
> > > + */
> > > +int create_card_sysfs(struct genwqe_dev *cd)
> > > +{
> > > + int rc, priv;
> > > + unsigned int i;
> > > +
> > > + priv = genwqe_is_privileged(cd);
> > > + for (i = 0; i < ARRAY_SIZE(dev_attr_tab); i++) {
> > > + struct genwqe_dev_attrib *dev_attr = _attr_tab[i];
> > > + if (dev_attr->vf || priv) {
> > > + rc = device_create_file(cd->dev, _attr->att);
> >
> > No, you just raced with userspace, creating the sysfs files _after_ it
> > was told to userspace that the driver was bound to the device.
>
> > Please use the attribute groups for this driver to have the driver core
> > create the files before it it told to userspace.
>
> The code got a little smaller now, which is good. I introduced the usage
> of is_visible(), but wondered if it really makes sense for my driver.
> Alternatively I thought I could use something like this:
>
> if (genwqe_is_privileged(cd))
> sysfs_create_group(>dev->kobj, _attribute_group);
> else
> sysfs_create_group(>dev->kobj,
>_normal_attribute_group);
Ideally you would never call that function, so yes, is_visible() should
be used.
> > Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
> > things easier to audit and saves me from having to change it later on
> > (I'm doing a tree-wide change of this type of thing...)
>
> Ok.
>
> I stumbled across your article:
> http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
>
> I am using sysfs_create_group() now, but do I understand you correctly
> that setting the const struct attribute_group **groups; in my device
> (where in my struct pci_device.dev?) is an even better way to establish
> my sysfs attributes? Is there a function which I could call rather than
> trying to find the right pointer?
Ugh, this is still a problem, I'm trying to work through how to have
individual drivers implement groups and sysfs files in a non-racy way.
The issue is your device was announced to userspace _before_ it was
bound to the driver, so there's no way to get the sysfs files to apply
before then.
You should just have the attributes on the sysfs device you create
yourself, not your pci device, as that's where they make more sense, and
there you should be able to use the attribute group easily, right?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Ryan, Am Donnerstag, den 31.10.2013, 14:49 +1100 schrieb Ryan Mallon: > > +/** > > + * create_card_sysfs() - Setup sysfs entries of the card device > > + * > > + * VFs have restricted mmio capabilities, so not all sysfs entries > > + * are allowed in VFs. > > + */ > > +int create_card_sysfs(struct genwqe_dev *cd) > > This is extern, and has a very generic name. Prefix it with genwqe or > something. Same for the function below, and for any other extern > symbols > you have. Good finding. I fixed that in the course of the other sysfs reworks too. Thanks for pointing it out. Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Ryan,
Am Donnerstag, den 31.10.2013, 14:49 +1100 schrieb Ryan Mallon:
> > +static ssize_t show_card_status(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + ssize_t len = 0;
> > + struct genwqe_dev *cd = dev_get_drvdata(dev);
> > + const char *cs[GENWQE_CARD_STATE_MAX] = { "unused", "used",
> "error" };
> > +
> > + len += scnprintf([len], PAGE_SIZE - len,
> > + "%s\n", cs[cd->card_state]);
> > + return len;
>
> This is a bit confusingly written. Why do:
>
> scnprintf([len], ...
>
> When len is always zero at that point? Since you are printing from an
> array of statically sized strings that are guaranteed to be smaller
> than
> PAGE_SIZE, you can safely do:
>
> sprintf(buf, "%s\n", cs[cd->card_state]);
>
> You might want to add some checking for cd->card_state being out of
> bounds, and printing "unknown" or something in that case.
>
> Same issue exists in other sysfs handlers.
>
I kept using scnprintf(), but reworked the code around it. ssize_t len
is not needed of course and the [len] stuff is obsolete too. I had
sysfs entries with multiple lines of output, where it was convenient and
safe to code it like that. Now since those interfaces were moved to
debugfs, the sysfs code can be simpler, as you pointed it out.
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg,
Am Mittwoch, den 30.10.2013, 10:44 -0700 schrieb Greg KH:
> On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
> > +/*
> > + * Create device_attribute structures / params: name, mode, show, store
> > + * additional flag if valid in VF
> > + */
> > +struct genwqe_dev_attrib {
> > + struct device_attribute att;/* sysfs entry attributes */
> > + int vf; /* may exist in VF or not */
> > +};
>
> Why do you need your own structure? Use the is_visible() callback to
> create or not, the individual attributes for a specific device, don't
> roll your own logic for something the driver core already supports.
>
> > +static struct genwqe_dev_attrib dev_attr_tab[] = {
> > + {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
> > + {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
> > + show_card_next_bitstream, store_card_next_bitstream), 0},
> > + {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
> > + {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
> > + {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
> > +
> > + {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
> > + {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
> > + {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
> > + {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
> > + {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
> > +};
> > +
> > +/**
> > + * create_card_sysfs() - Setup sysfs entries of the card device
> > + *
> > + * VFs have restricted mmio capabilities, so not all sysfs entries
> > + * are allowed in VFs.
> > + */
> > +int create_card_sysfs(struct genwqe_dev *cd)
> > +{
> > + int rc, priv;
> > + unsigned int i;
> > +
> > + priv = genwqe_is_privileged(cd);
> > + for (i = 0; i < ARRAY_SIZE(dev_attr_tab); i++) {
> > + struct genwqe_dev_attrib *dev_attr = _attr_tab[i];
> > + if (dev_attr->vf || priv) {
> > + rc = device_create_file(cd->dev, _attr->att);
>
> No, you just raced with userspace, creating the sysfs files _after_ it
> was told to userspace that the driver was bound to the device.
> Please use the attribute groups for this driver to have the driver core
> create the files before it it told to userspace.
The code got a little smaller now, which is good. I introduced the usage
of is_visible(), but wondered if it really makes sense for my driver.
Alternatively I thought I could use something like this:
if (genwqe_is_privileged(cd))
sysfs_create_group(>dev->kobj, _attribute_group);
else
sysfs_create_group(>dev->kobj,
_normal_attribute_group);
> Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
> things easier to audit and saves me from having to change it later on
> (I'm doing a tree-wide change of this type of thing...)
Ok.
I stumbled across your article:
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
I am using sysfs_create_group() now, but do I understand you correctly
that setting the const struct attribute_group **groups; in my device
(where in my struct pci_device.dev?) is an even better way to establish
my sysfs attributes? Is there a function which I could call rather than
trying to find the right pointer?
> thanks,
>
> greg k-h
>
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:37 -0700 schrieb Greg KH: > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > > +/* > > + * We like to be able to disable the health checking entirely in some > > + * cases e.g. if a card is broken and needs to be analyzed. I > > + * considered using debugfs/sysfs attributes, but I did not see a way > > + * to prevent the thread from being started at driver load time other > > + * than starting it later manually, what I did not like either > > + * in this case (e.g. like the new SRIOV enablement). > > + */ > > +static int genwqe_health_check_interval = 4; /* <= 0: disabled */ > > +module_param(genwqe_health_check_interval, int, 0644); /* read/writeable > > */ > > +MODULE_PARM_DESC(genwqe_health_check_interval, > > +"check card health every N seconds (0 = disabled)"); > > Module paramaters are driver wide, not device specific. > > If a card is broken, it's broken, send it back. No one will ever know > to set this value, and then you just affected all other cards in the > system for this same driver. > > Just delete this please. > > thanks, > > greg k-h > Finally I removed all my module parameters or replaced them by debugfs or sysfs attributes. Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Am Donnerstag, den 31.10.2013, 07:38 +1100 schrieb Ryan Mallon: > On 31/10/13 04:35, Greg KH wrote: > > > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > >> +if GENWQE > >> + > >> +config GENWQE_DEVNAME > >> +string "Name for sysfs and device nodes" > >> + default "genwqe" > >> +help > >> + Select alternate name for sysfs and device nodes. > > > > Don't let the user pick this, it's up to the driver to set this once and > > then live with it. > > > > And, from what I can tell in the driver, this help text is wrong (there > > are no device nodes with this name in it, right?) > > > The indentation is also messed up here. Kconfig should use hard tabs, with > two additional spaces for the help text. This has mixed tabs and spaces > between lines. > Ok. Removed it anyways. > ~Ryan > > Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:35 -0700 schrieb Greg KH: > Don't let the user pick this, it's up to the driver to set this once > and > then live with it. Ok. I will remove it in the next version. > > And, from what I can tell in the driver, this help text is wrong > (there > are no device nodes with this name in it, right?) There is a /dev/genwqe_card character device: card_dev.c: cd->dev = device_create(cd->class_genwqe, >pci_dev->dev, cd->devnum_genwqe, NULL, GENWQE_DEVNAME "%u_card", cd->card_idx); That is what I meant with "device node". Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:36 -0700 schrieb Greg KH: > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > > +/* > > + * Module initialization and PCIe setup. Card health monitoring and > > + * recovery functionality. Character device creation and deletion are > > + * controlled from here. > > + * > > + * Please use the new sysfs interfaces to enable the VFs after PF > > + * driver loading. > > + * > > + * Enable VFs: > > + * sudo sh -c 'echo 15 > > > /sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs' > > + * or > > + * sudo sh -c 'echo 15 > > > /sys/class/genwqe/genwqe0_card/device/sriov_numvfs' > > + * Disable VFs: > > + * Write a 0 into the same sysfs entries. > > + */ > > Put this documentation somewhere a user has a _chance_ of reading it. > Digging it out of code comments is not ok. Why isn't this in the ABI > file for the sysfs files? > > thanks, > > greg k-h > True. There was other stuff I moved/deleted now too. Regards Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:35 -0700 schrieb Greg KH: Don't let the user pick this, it's up to the driver to set this once and then live with it. Ok. I will remove it in the next version. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) There is a /dev/genwqen_card character device: card_dev.c: cd-dev = device_create(cd-class_genwqe, cd-pci_dev-dev, cd-devnum_genwqe, NULL, GENWQE_DEVNAME %u_card, cd-card_idx); That is what I meant with device node. Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Am Donnerstag, den 31.10.2013, 07:38 +1100 schrieb Ryan Mallon: On 31/10/13 04:35, Greg KH wrote: On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +if GENWQE + +config GENWQE_DEVNAME +string Name for sysfs and device nodes + default genwqe +help + Select alternate name for sysfs and device nodes. Don't let the user pick this, it's up to the driver to set this once and then live with it. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) The indentation is also messed up here. Kconfig should use hard tabs, with two additional spaces for the help text. This has mixed tabs and spaces between lines. Ok. Removed it anyways. ~Ryan Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:37 -0700 schrieb Greg KH: On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +/* + * We like to be able to disable the health checking entirely in some + * cases e.g. if a card is broken and needs to be analyzed. I + * considered using debugfs/sysfs attributes, but I did not see a way + * to prevent the thread from being started at driver load time other + * than starting it later manually, what I did not like either + * in this case (e.g. like the new SRIOV enablement). + */ +static int genwqe_health_check_interval = 4; /* = 0: disabled */ +module_param(genwqe_health_check_interval, int, 0644); /* read/writeable */ +MODULE_PARM_DESC(genwqe_health_check_interval, +check card health every N seconds (0 = disabled)); Module paramaters are driver wide, not device specific. If a card is broken, it's broken, send it back. No one will ever know to set this value, and then you just affected all other cards in the system for this same driver. Just delete this please. thanks, greg k-h Finally I removed all my module parameters or replaced them by debugfs or sysfs attributes. Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg,
Am Mittwoch, den 30.10.2013, 10:44 -0700 schrieb Greg KH:
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
+/*
+ * Create device_attribute structures / params: name, mode, show, store
+ * additional flag if valid in VF
+ */
+struct genwqe_dev_attrib {
+ struct device_attribute att;/* sysfs entry attributes */
+ int vf; /* may exist in VF or not */
+};
Why do you need your own structure? Use the is_visible() callback to
create or not, the individual attributes for a specific device, don't
roll your own logic for something the driver core already supports.
+static struct genwqe_dev_attrib dev_attr_tab[] = {
+ {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
+ {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
+ show_card_next_bitstream, store_card_next_bitstream), 0},
+ {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
+ {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
+ {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
+
+ {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
+ {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
+ {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
+ {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
+ {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
+};
+
+/**
+ * create_card_sysfs() - Setup sysfs entries of the card device
+ *
+ * VFs have restricted mmio capabilities, so not all sysfs entries
+ * are allowed in VFs.
+ */
+int create_card_sysfs(struct genwqe_dev *cd)
+{
+ int rc, priv;
+ unsigned int i;
+
+ priv = genwqe_is_privileged(cd);
+ for (i = 0; i ARRAY_SIZE(dev_attr_tab); i++) {
+ struct genwqe_dev_attrib *dev_attr = dev_attr_tab[i];
+ if (dev_attr-vf || priv) {
+ rc = device_create_file(cd-dev, dev_attr-att);
No, you just raced with userspace, creating the sysfs files _after_ it
was told to userspace that the driver was bound to the device.
Please use the attribute groups for this driver to have the driver core
create the files before it it told to userspace.
The code got a little smaller now, which is good. I introduced the usage
of is_visible(), but wondered if it really makes sense for my driver.
Alternatively I thought I could use something like this:
if (genwqe_is_privileged(cd))
sysfs_create_group(cd-dev-kobj, genwqe_attribute_group);
else
sysfs_create_group(cd-dev-kobj,
genwqe_normal_attribute_group);
Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
things easier to audit and saves me from having to change it later on
(I'm doing a tree-wide change of this type of thing...)
Ok.
I stumbled across your article:
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
I am using sysfs_create_group() now, but do I understand you correctly
that setting the const struct attribute_group **groups; in my device
(where in my struct pci_device.dev?) is an even better way to establish
my sysfs attributes? Is there a function which I could call rather than
trying to find the right pointer?
thanks,
greg k-h
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Ryan,
Am Donnerstag, den 31.10.2013, 14:49 +1100 schrieb Ryan Mallon:
+static ssize_t show_card_status(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t len = 0;
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+ const char *cs[GENWQE_CARD_STATE_MAX] = { unused, used,
error };
+
+ len += scnprintf(buf[len], PAGE_SIZE - len,
+ %s\n, cs[cd-card_state]);
+ return len;
This is a bit confusingly written. Why do:
scnprintf(buf[len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller
than
PAGE_SIZE, you can safely do:
sprintf(buf, %s\n, cs[cd-card_state]);
You might want to add some checking for cd-card_state being out of
bounds, and printing unknown or something in that case.
Same issue exists in other sysfs handlers.
I kept using scnprintf(), but reworked the code around it. ssize_t len
is not needed of course and the buf[len] stuff is obsolete too. I had
sysfs entries with multiple lines of output, where it was convenient and
safe to code it like that. Now since those interfaces were moved to
debugfs, the sysfs code can be simpler, as you pointed it out.
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Ryan, Am Donnerstag, den 31.10.2013, 14:49 +1100 schrieb Ryan Mallon: +/** + * create_card_sysfs() - Setup sysfs entries of the card device + * + * VFs have restricted mmio capabilities, so not all sysfs entries + * are allowed in VFs. + */ +int create_card_sysfs(struct genwqe_dev *cd) This is extern, and has a very generic name. Prefix it with genwqe or something. Same for the function below, and for any other extern symbols you have. Good finding. I fixed that in the course of the other sysfs reworks too. Thanks for pointing it out. Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Mon, Nov 04, 2013 at 05:41:27PM +0100, Frank Haverkamp wrote:
Hi Greg,
Am Mittwoch, den 30.10.2013, 10:44 -0700 schrieb Greg KH:
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
+/*
+ * Create device_attribute structures / params: name, mode, show, store
+ * additional flag if valid in VF
+ */
+struct genwqe_dev_attrib {
+ struct device_attribute att;/* sysfs entry attributes */
+ int vf; /* may exist in VF or not */
+};
Why do you need your own structure? Use the is_visible() callback to
create or not, the individual attributes for a specific device, don't
roll your own logic for something the driver core already supports.
+static struct genwqe_dev_attrib dev_attr_tab[] = {
+ {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
+ {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
+ show_card_next_bitstream, store_card_next_bitstream), 0},
+ {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
+ {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
+ {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
+
+ {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
+ {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
+ {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
+ {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
+ {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
+};
+
+/**
+ * create_card_sysfs() - Setup sysfs entries of the card device
+ *
+ * VFs have restricted mmio capabilities, so not all sysfs entries
+ * are allowed in VFs.
+ */
+int create_card_sysfs(struct genwqe_dev *cd)
+{
+ int rc, priv;
+ unsigned int i;
+
+ priv = genwqe_is_privileged(cd);
+ for (i = 0; i ARRAY_SIZE(dev_attr_tab); i++) {
+ struct genwqe_dev_attrib *dev_attr = dev_attr_tab[i];
+ if (dev_attr-vf || priv) {
+ rc = device_create_file(cd-dev, dev_attr-att);
No, you just raced with userspace, creating the sysfs files _after_ it
was told to userspace that the driver was bound to the device.
Please use the attribute groups for this driver to have the driver core
create the files before it it told to userspace.
The code got a little smaller now, which is good. I introduced the usage
of is_visible(), but wondered if it really makes sense for my driver.
Alternatively I thought I could use something like this:
if (genwqe_is_privileged(cd))
sysfs_create_group(cd-dev-kobj, genwqe_attribute_group);
else
sysfs_create_group(cd-dev-kobj,
genwqe_normal_attribute_group);
Ideally you would never call that function, so yes, is_visible() should
be used.
Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
things easier to audit and saves me from having to change it later on
(I'm doing a tree-wide change of this type of thing...)
Ok.
I stumbled across your article:
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
I am using sysfs_create_group() now, but do I understand you correctly
that setting the const struct attribute_group **groups; in my device
(where in my struct pci_device.dev?) is an even better way to establish
my sysfs attributes? Is there a function which I could call rather than
trying to find the right pointer?
Ugh, this is still a problem, I'm trying to work through how to have
individual drivers implement groups and sysfs files in a non-racy way.
The issue is your device was announced to userspace _before_ it was
bound to the driver, so there's no way to get the sysfs files to apply
before then.
You should just have the attributes on the sysfs device you create
yourself, not your pci device, as that's where they make more sense, and
there you should be able to use the attribute group easily, right?
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Montag, den 04.11.2013, 14:15 -0800 schrieb Greg KH: I am using sysfs_create_group() now, but do I understand you correctly that setting the const struct attribute_group **groups; in my device (where in my struct pci_device.dev?) is an even better way to establish my sysfs attributes? Is there a function which I could call rather than trying to find the right pointer? Ugh, this is still a problem, I'm trying to work through how to have individual drivers implement groups and sysfs files in a non-racy way. The issue is your device was announced to userspace _before_ it was bound to the driver, so there's no way to get the sysfs files to apply before then. You should just have the attributes on the sysfs device you create yourself, not your pci device, as that's where they make more sense, and there you should be able to use the attribute group easily, right? The current version works well for me and I did not get any complaints about the possible race-condition you mentioned so far. So I am happy. thanks, greg k-h Greetings Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
Hi Greg, Am Mittwoch, den 30.10.2013, 10:36 -0700 schrieb Greg KH: On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +/* + * Module initialization and PCIe setup. Card health monitoring and + * recovery functionality. Character device creation and deletion are + * controlled from here. + * + * Please use the new sysfs interfaces to enable the VFs after PF + * driver loading. + * + * Enable VFs: + * sudo sh -c 'echo 15 /sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs' + * or + * sudo sh -c 'echo 15 /sys/class/genwqe/genwqe0_card/device/sriov_numvfs' + * Disable VFs: + * Write a 0 into the same sysfs entries. + */ Put this documentation somewhere a user has a _chance_ of reading it. Digging it out of code comments is not ok. Why isn't this in the ABI file for the sysfs files? thanks, greg k-h True. There was other stuff I moved/deleted now too. Regards Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 30/10/13 20:32, Frank Haverkamp wrote:
> From: Frank Haverkamp
> Signed-off-by: Frank Haverkamp
> Co-authors: Joerg-Stephan Vogt ,
> Michael Jung ,
> Michael Ruettger
> ---
> Documentation/ABI/testing/debugfs-driver-genwqe | 57 +
> Documentation/ABI/testing/sysfs-driver-genwqe | 46 +
> drivers/misc/Kconfig|1 +
> drivers/misc/Makefile |1 +
> drivers/misc/genwqe/Kconfig | 23 +
> drivers/misc/genwqe/Makefile|8 +
> drivers/misc/genwqe/card_base.c | 1238 +++
> drivers/misc/genwqe/card_base.h | 569 +
> drivers/misc/genwqe/card_ddcb.c | 1380 +
> drivers/misc/genwqe/card_ddcb.h | 188 +++
> drivers/misc/genwqe/card_debugfs.c | 566 +
> drivers/misc/genwqe/card_dev.c | 1499
> +++
> drivers/misc/genwqe/card_sysfs.c| 306 +
> drivers/misc/genwqe/card_utils.c| 983 +++
> drivers/misc/genwqe/genwqe_driver.h | 75 ++
> include/linux/genwqe/genwqe_card.h | 559 +
> 16 files changed, 7499 insertions(+), 0 deletions(-)
Please consider breaking this into multiple patches. One behemoth patch
is harder to review and less likely for people to read all of it. Since
this adds a new driver, you can add it in parts, e.g: core, sysfs,
documentation, etc, and add the Makefile/Kconfig bits in the final
patch. That way it won't break the build at any point.
Couple of comments on the sysfs bits below.
~Ryan
> +++ b/drivers/misc/genwqe/card_sysfs.c
> @@ -0,0 +1,306 @@
> +/**
> + * IBM Accelerator Family 'GenWQE'
> + *
> + * (C) Copyright IBM Corp. 2013
> + *
> + * Author: Frank Haverkamp
> + * Author: Joerg-Stephan Vogt
> + * Author: Michael Jung
> + * Author: Michael Ruettger
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2 only)
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * Sysfs interfaces for the GenWQE card. There are attributes to query
> + * the version of the bitstream as well as some for the
> + * driver. Additionally there are some attributes which help to debug
> + * potential problems.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "card_base.h"
> +#include "card_ddcb.h"
> +
> +static const char * const genwqe_types[] = {
> + [GENWQE_TYPE_ALTERA_230] = "GenWQE4-230",
> + [GENWQE_TYPE_ALTERA_530] = "GenWQE4-530",
> + [GENWQE_TYPE_ALTERA_A4] = "GenWQE5-A4",
> + [GENWQE_TYPE_ALTERA_A7] = "GenWQE5-A7",
> +};
> +
> +static ssize_t show_card_status(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> + const char *cs[GENWQE_CARD_STATE_MAX] = { "unused", "used", "error" };
> +
> + len += scnprintf([len], PAGE_SIZE - len,
> + "%s\n", cs[cd->card_state]);
> + return len;
This is a bit confusingly written. Why do:
scnprintf([len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller than
PAGE_SIZE, you can safely do:
sprintf(buf, "%s\n", cs[cd->card_state]);
You might want to add some checking for cd->card_state being out of
bounds, and printing "unknown" or something in that case.
Same issue exists in other sysfs handlers.
> +}
> +
> +static ssize_t show_card_appid(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + ssize_t len = 0;
> + char app_name[5];
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + genwqe_read_app_id(cd, app_name, sizeof(app_name));
> + len += scnprintf([len], PAGE_SIZE - len,
> + "%s\n", app_name);
> + return len;
> +}
> +
> +static ssize_t show_card_version(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + u64 slu_id, app_id;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + slu_id = __genwqe_readq(cd, IO_SLU_UNITCFG);
> + app_id = __genwqe_readq(cd, IO_APP_UNITCFG);
> +
> + len += scnprintf([len], PAGE_SIZE - len,
> +
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 31/10/13 04:35, Greg KH wrote: > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: >> +if GENWQE >> + >> +config GENWQE_DEVNAME >> +string "Name for sysfs and device nodes" >> +default "genwqe" >> +help >> + Select alternate name for sysfs and device nodes. > > Don't let the user pick this, it's up to the driver to set this once and > then live with it. > > And, from what I can tell in the driver, this help text is wrong (there > are no device nodes with this name in it, right?) The indentation is also messed up here. Kconfig should use hard tabs, with two additional spaces for the help text. This has mixed tabs and spaces between lines. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
> +/*
> + * Create device_attribute structures / params: name, mode, show, store
> + * additional flag if valid in VF
> + */
> +struct genwqe_dev_attrib {
> + struct device_attribute att;/* sysfs entry attributes */
> + int vf; /* may exist in VF or not */
> +};
Why do you need your own structure? Use the is_visible() callback to
create or not, the individual attributes for a specific device, don't
roll your own logic for something the driver core already supports.
> +static struct genwqe_dev_attrib dev_attr_tab[] = {
> + {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
> + {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
> + show_card_next_bitstream, store_card_next_bitstream), 0},
> + {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
> + {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
> + {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
> +
> + {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
> + {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
> + {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
> + {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
> + {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
> +};
> +
> +/**
> + * create_card_sysfs() - Setup sysfs entries of the card device
> + *
> + * VFs have restricted mmio capabilities, so not all sysfs entries
> + * are allowed in VFs.
> + */
> +int create_card_sysfs(struct genwqe_dev *cd)
> +{
> + int rc, priv;
> + unsigned int i;
> +
> + priv = genwqe_is_privileged(cd);
> + for (i = 0; i < ARRAY_SIZE(dev_attr_tab); i++) {
> + struct genwqe_dev_attrib *dev_attr = _attr_tab[i];
> + if (dev_attr->vf || priv) {
> + rc = device_create_file(cd->dev, _attr->att);
No, you just raced with userspace, creating the sysfs files _after_ it
was told to userspace that the driver was bound to the device.
Please use the attribute groups for this driver to have the driver core
create the files before it it told to userspace.
Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
things easier to audit and saves me from having to change it later on
(I'm doing a tree-wide change of this type of thing...)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > +/* > + * GenWQE Driver: Need SLC timeout set to 250ms (temporary setting for > + * testing of 1000ms due to decompressor testcase failing) > + * > + * There is a requirement by the card users that the timeout must not > + * exceed the 250ms. > + * > + * In this case the settings must be done before any interaction to > + * the device can be done, since we cannot change the settings without > + * stopping the queues. Then stop the queue, change the value, and start them up again. Why would anyone ever want to change these values? Don't make anything "tunable", just get it right and don't rely on someone making tweaks later with module options, especially as, again, these are driver wide, not specific to a device. If you _really_ need them for a device, make them a sysfs file and fix the code to handle stopping and starting. You have to handle that anyway with dynamic bind/unbind of the driver to specific devices anyway, so this shouldn't be a big deal. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > +/* > + * We like to be able to disable the health checking entirely in some > + * cases e.g. if a card is broken and needs to be analyzed. I > + * considered using debugfs/sysfs attributes, but I did not see a way > + * to prevent the thread from being started at driver load time other > + * than starting it later manually, what I did not like either > + * in this case (e.g. like the new SRIOV enablement). > + */ > +static int genwqe_health_check_interval = 4; /* <= 0: disabled */ > +module_param(genwqe_health_check_interval, int, 0644); /* read/writeable */ > +MODULE_PARM_DESC(genwqe_health_check_interval, > + "check card health every N seconds (0 = disabled)"); Module paramaters are driver wide, not device specific. If a card is broken, it's broken, send it back. No one will ever know to set this value, and then you just affected all other cards in the system for this same driver. Just delete this please. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > +/* > + * Module initialization and PCIe setup. Card health monitoring and > + * recovery functionality. Character device creation and deletion are > + * controlled from here. > + * > + * Please use the new sysfs interfaces to enable the VFs after PF > + * driver loading. > + * > + * Enable VFs: > + * sudo sh -c 'echo 15 > /sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs' > + * or > + * sudo sh -c 'echo 15 > > /sys/class/genwqe/genwqe0_card/device/sriov_numvfs' > + * Disable VFs: > + * Write a 0 into the same sysfs entries. > + */ Put this documentation somewhere a user has a _chance_ of reading it. Digging it out of code comments is not ok. Why isn't this in the ABI file for the sysfs files? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: > +if GENWQE > + > +config GENWQE_DEVNAME > +string "Name for sysfs and device nodes" > + default "genwqe" > +help > + Select alternate name for sysfs and device nodes. Don't let the user pick this, it's up to the driver to set this once and then live with it. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +if GENWQE + +config GENWQE_DEVNAME +string Name for sysfs and device nodes + default genwqe +help + Select alternate name for sysfs and device nodes. Don't let the user pick this, it's up to the driver to set this once and then live with it. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +/* + * Module initialization and PCIe setup. Card health monitoring and + * recovery functionality. Character device creation and deletion are + * controlled from here. + * + * Please use the new sysfs interfaces to enable the VFs after PF + * driver loading. + * + * Enable VFs: + * sudo sh -c 'echo 15 /sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs' + * or + * sudo sh -c 'echo 15 /sys/class/genwqe/genwqe0_card/device/sriov_numvfs' + * Disable VFs: + * Write a 0 into the same sysfs entries. + */ Put this documentation somewhere a user has a _chance_ of reading it. Digging it out of code comments is not ok. Why isn't this in the ABI file for the sysfs files? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +/* + * We like to be able to disable the health checking entirely in some + * cases e.g. if a card is broken and needs to be analyzed. I + * considered using debugfs/sysfs attributes, but I did not see a way + * to prevent the thread from being started at driver load time other + * than starting it later manually, what I did not like either + * in this case (e.g. like the new SRIOV enablement). + */ +static int genwqe_health_check_interval = 4; /* = 0: disabled */ +module_param(genwqe_health_check_interval, int, 0644); /* read/writeable */ +MODULE_PARM_DESC(genwqe_health_check_interval, + check card health every N seconds (0 = disabled)); Module paramaters are driver wide, not device specific. If a card is broken, it's broken, send it back. No one will ever know to set this value, and then you just affected all other cards in the system for this same driver. Just delete this please. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +/* + * GenWQE Driver: Need SLC timeout set to 250ms (temporary setting for + * testing of 1000ms due to decompressor testcase failing) + * + * There is a requirement by the card users that the timeout must not + * exceed the 250ms. + * + * In this case the settings must be done before any interaction to + * the device can be done, since we cannot change the settings without + * stopping the queues. Then stop the queue, change the value, and start them up again. Why would anyone ever want to change these values? Don't make anything tunable, just get it right and don't rely on someone making tweaks later with module options, especially as, again, these are driver wide, not specific to a device. If you _really_ need them for a device, make them a sysfs file and fix the code to handle stopping and starting. You have to handle that anyway with dynamic bind/unbind of the driver to specific devices anyway, so this shouldn't be a big deal. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote:
+/*
+ * Create device_attribute structures / params: name, mode, show, store
+ * additional flag if valid in VF
+ */
+struct genwqe_dev_attrib {
+ struct device_attribute att;/* sysfs entry attributes */
+ int vf; /* may exist in VF or not */
+};
Why do you need your own structure? Use the is_visible() callback to
create or not, the individual attributes for a specific device, don't
roll your own logic for something the driver core already supports.
+static struct genwqe_dev_attrib dev_attr_tab[] = {
+ {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
+ {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
+ show_card_next_bitstream, store_card_next_bitstream), 0},
+ {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
+ {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
+ {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
+
+ {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
+ {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
+ {__ATTR(version,S_IRUGO, show_card_version, NULL), 1},
+ {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
+ {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
+};
+
+/**
+ * create_card_sysfs() - Setup sysfs entries of the card device
+ *
+ * VFs have restricted mmio capabilities, so not all sysfs entries
+ * are allowed in VFs.
+ */
+int create_card_sysfs(struct genwqe_dev *cd)
+{
+ int rc, priv;
+ unsigned int i;
+
+ priv = genwqe_is_privileged(cd);
+ for (i = 0; i ARRAY_SIZE(dev_attr_tab); i++) {
+ struct genwqe_dev_attrib *dev_attr = dev_attr_tab[i];
+ if (dev_attr-vf || priv) {
+ rc = device_create_file(cd-dev, dev_attr-att);
No, you just raced with userspace, creating the sysfs files _after_ it
was told to userspace that the driver was bound to the device.
Please use the attribute groups for this driver to have the driver core
create the files before it it told to userspace.
Also, instead of using __ATTR(), please use DEVICE_ATTR_RO(), it makes
things easier to audit and saves me from having to change it later on
(I'm doing a tree-wide change of this type of thing...)
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 31/10/13 04:35, Greg KH wrote: On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +if GENWQE + +config GENWQE_DEVNAME +string Name for sysfs and device nodes +default genwqe +help + Select alternate name for sysfs and device nodes. Don't let the user pick this, it's up to the driver to set this once and then live with it. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) The indentation is also messed up here. Kconfig should use hard tabs, with two additional spaces for the help text. This has mixed tabs and spaces between lines. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 30/10/13 20:32, Frank Haverkamp wrote:
From: Frank Haverkamp ha...@vnet.ibm.com
Signed-off-by: Frank Haverkamp ha...@linux.vnet.ibm.com
Co-authors: Joerg-Stephan Vogt jsv...@de.ibm.com,
Michael Jung mij...@de.ibm.com,
Michael Ruettger mich...@ibmra.de
---
Documentation/ABI/testing/debugfs-driver-genwqe | 57 +
Documentation/ABI/testing/sysfs-driver-genwqe | 46 +
drivers/misc/Kconfig|1 +
drivers/misc/Makefile |1 +
drivers/misc/genwqe/Kconfig | 23 +
drivers/misc/genwqe/Makefile|8 +
drivers/misc/genwqe/card_base.c | 1238 +++
drivers/misc/genwqe/card_base.h | 569 +
drivers/misc/genwqe/card_ddcb.c | 1380 +
drivers/misc/genwqe/card_ddcb.h | 188 +++
drivers/misc/genwqe/card_debugfs.c | 566 +
drivers/misc/genwqe/card_dev.c | 1499
+++
drivers/misc/genwqe/card_sysfs.c| 306 +
drivers/misc/genwqe/card_utils.c| 983 +++
drivers/misc/genwqe/genwqe_driver.h | 75 ++
include/linux/genwqe/genwqe_card.h | 559 +
16 files changed, 7499 insertions(+), 0 deletions(-)
Please consider breaking this into multiple patches. One behemoth patch
is harder to review and less likely for people to read all of it. Since
this adds a new driver, you can add it in parts, e.g: core, sysfs,
documentation, etc, and add the Makefile/Kconfig bits in the final
patch. That way it won't break the build at any point.
Couple of comments on the sysfs bits below.
~Ryan
+++ b/drivers/misc/genwqe/card_sysfs.c
@@ -0,0 +1,306 @@
+/**
+ * IBM Accelerator Family 'GenWQE'
+ *
+ * (C) Copyright IBM Corp. 2013
+ *
+ * Author: Frank Haverkamp ha...@linux.vnet.ibm.com
+ * Author: Joerg-Stephan Vogt jsv...@de.ibm.com
+ * Author: Michael Jung mij...@de.ibm.com
+ * Author: Michael Ruettger mich...@ibmra.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2 only)
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * Sysfs interfaces for the GenWQE card. There are attributes to query
+ * the version of the bitstream as well as some for the
+ * driver. Additionally there are some attributes which help to debug
+ * potential problems.
+ */
+
+#include linux/version.h
+#include linux/kernel.h
+#include linux/types.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/string.h
+#include linux/fs.h
+#include linux/sysfs.h
+#include linux/ctype.h
+
+#include card_base.h
+#include card_ddcb.h
+
+static const char * const genwqe_types[] = {
+ [GENWQE_TYPE_ALTERA_230] = GenWQE4-230,
+ [GENWQE_TYPE_ALTERA_530] = GenWQE4-530,
+ [GENWQE_TYPE_ALTERA_A4] = GenWQE5-A4,
+ [GENWQE_TYPE_ALTERA_A7] = GenWQE5-A7,
+};
+
+static ssize_t show_card_status(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t len = 0;
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+ const char *cs[GENWQE_CARD_STATE_MAX] = { unused, used, error };
+
+ len += scnprintf(buf[len], PAGE_SIZE - len,
+ %s\n, cs[cd-card_state]);
+ return len;
This is a bit confusingly written. Why do:
scnprintf(buf[len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller than
PAGE_SIZE, you can safely do:
sprintf(buf, %s\n, cs[cd-card_state]);
You might want to add some checking for cd-card_state being out of
bounds, and printing unknown or something in that case.
Same issue exists in other sysfs handlers.
+}
+
+static ssize_t show_card_appid(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+ ssize_t len = 0;
+ char app_name[5];
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+
+ genwqe_read_app_id(cd, app_name, sizeof(app_name));
+ len += scnprintf(buf[len], PAGE_SIZE - len,
+ %s\n, app_name);
+ return len;
+}
+
+static ssize_t show_card_version(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t len = 0;
+ u64 slu_id, app_id;
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+
+ slu_id =
