On Fri, Oct 24, 2014 at 3:54 PM, atull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 24 Oct 2014, Pantelis Antoniou wrote: > >> Hi Pavel, >> >> > On Oct 24, 2014, at 1:52 PM, Pavel Machek <pavel@xxxxxxx> wrote: >> > >> > Hi! >> > >> >> * /sys/class/fpga_manager/<fpga>/firmware >> >> Name of FPGA image file to load using firmware class. >> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware >> > >> > I .. still don't think this is good idea. What about namespaces? >> > The path corresponds to path in which namespace? >> > > > Hi Pavel, > > Sorry if my documentation is too brief here. The context is the > firmware class. The 'image.rbf' is a image file that is located > on the filesystem in the firmware search patch. > See Documentation/firmware_class/README. > > Basically we need some way of loading a FPGA image to the FPGA. > I don't think any one way is going to meet everybody's needs so > I wanted to export enough functions from fpga-mgr.c that other > interfaces can by built. I think firmware will work just fine > for some people and it is great in that it already exists in the > kernel. > > If I missed your point, please let me know. > >> >> FWIW the overlays patchset uses binary configfs attribute to make this work. >> > > Hi Pantelis, > > Yes I think you've mentioned that before. So that would be another > way of giving a device tree overlay to configfs, right? I should > that to the documentation in the patch header. > >> >> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count) >> >> +{ >> >> + int ret; >> >> + >> >> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags)) >> >> + return -EBUSY; >> >> + >> >> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name); >> >> + >> >> + ret = __fpga_mgr_write(mgr, buf, count); >> >> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags); >> >> + >> >> + return ret; >> >> +} >> >> +EXPORT_SYMBOL_GPL(fpga_mgr_write); >> > >> > Is the EBUSY -- userspace please try again, but you don't know when to >> > try again -- right interface? I mean, normally kernel would wait, so >> > that userland does not have to poll? >> > > > Yes, for fpga_mgr_write it may be more useful for this to be > a blocking call. > >> >> +static ssize_t fpga_mgr_firmware_store(struct device *dev, >> >> + struct device_attribute *attr, >> >> + const char *buf, size_t count) >> >> +{ >> >> + struct fpga_manager *mgr = dev_get_drvdata(dev); >> >> + unsigned int len; >> >> + char image_name[NAME_MAX]; >> >> + int ret; >> >> + >> >> + /* lose terminating \n */ >> >> + strcpy(image_name, buf); >> >> + len = strlen(image_name); >> >> + if (image_name[len - 1] == '\n') >> >> + image_name[len - 1] = 0; >> >> + >> >> + ret = fpga_mgr_firmware_write(mgr, image_name); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + return count; >> >> +} >> > >> > This shows why the interface is not right... Valid filename may >> > contain \n, right? It may even end with \n. >> > >> >> I could argue that a valid firmware file is one that's well formed and does not >> contain those characters. >> >> I guess this is only for make echo file >foo work. You could specify that echo -n file >foo is >> required. >> > > I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a > while I am a bonehead and forget the '-n' and spend a few minutes > wondering why this thing that worked last week suddenly rejects all > commands. I'm just trying to make my user interface a bit user-friendly. > > I will take out the '\n' stripping and update the documentation. I didn't > realize this would be controversial. Don't. You're doing the right thing by scrubbing your input. Requiring 'echo -n' is just stupid when it is so easy to make work easily. g. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html