Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 30, 2015 at 2:17 AM, Kweh, Hock Leong wrote: >> -Original Message- >> From: Andy Lutomirski [mailto:l...@amacapital.net] >> Sent: Thursday, April 30, 2015 2:41 AM >> >> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong >> wrote: >> > >> > Dear communities, >> > >> > I agree with James. Due to different people may have different needs. >> > But from our side, we would just like to have a simple interface for >> > us to upload the efi capsule and perform update. We do not have any >> > use case or need to get info from QueryCapsuleUpdate(). Let me give a >> suggestion here: >> > please allow me to focus on deliver this simple loading interface and >> > upstream it. Then later whoever has the actual use case or needs on >> > the ioctl implementation, he or she could enhance base on this simple >> loading interface. >> > What do you guys think? >> > >> > Let me summarize the latest design idea: >> > - No longer leverage on firmware class but use misc device >> > - Do not use platform device but use device_create() >> > - User just need to perform "cat file.bin > /sys/.../capsule_loader" >> > in the shell >> >> If you do this, there's no need for the misc device. > > I do this so that in the future when someone want to implement the > Ioctl(), he or she can base on this and expand it. > >> >> > - File operation functions include: open(), read(), write() and >> > flush() >> > - Perform mutex lock in open() then release the mutex in flush() for >> avoiding >> >race condition / concurrent loading >> >> Make sure the mutex operation is killable, then, and maybe even >> interruptable. > > Okay. > >> >> > - Perform the capsule update and error return at flush() function >> > >> > Is there anything I missed? Any one still have concern with this idea? >> > Thanks for providing the ideas as well as the review. >> > >> >> If it works (and cat really does fail reliably), then it seems okay to me. >> >> However, since I like pulling increasing numbers of my hats, someone should >> verify that the common embedded cat implementations are also okay with >> this. For example, I haven't yet found any code in busybox's cat >> implementation that closes stdout. >> >> Given that the main targets of this (for now, at least) are embedded, this >> might be a problem. >> > > I think we shouldn't focus on the cat implementation for the close issue. > > My understanding about this action: > cat file.bin > /sys//capsule_loader > It is actually the ">" (IO redirection) who perform the open write & close > to this "/sys//capsule_loader" file note and not the "cat" do it. > So, I think your answer can be found at Shell source code. The shell opens capsule_loader and then execs the command. If you type: (cat file.bin) >/sys/.../captule_loader, then the command is a subshell and the subshell will close the file. (cat might also close it, but there will be two references.) If you type: cat file.bin >/sys/.../capsule_loader then the shell doesn't retain a reference to the file at all. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Thursday, April 30, 2015 2:41 AM > > On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong > wrote: > > > > Dear communities, > > > > I agree with James. Due to different people may have different needs. > > But from our side, we would just like to have a simple interface for > > us to upload the efi capsule and perform update. We do not have any > > use case or need to get info from QueryCapsuleUpdate(). Let me give a > suggestion here: > > please allow me to focus on deliver this simple loading interface and > > upstream it. Then later whoever has the actual use case or needs on > > the ioctl implementation, he or she could enhance base on this simple > loading interface. > > What do you guys think? > > > > Let me summarize the latest design idea: > > - No longer leverage on firmware class but use misc device > > - Do not use platform device but use device_create() > > - User just need to perform "cat file.bin > /sys/.../capsule_loader" > > in the shell > > If you do this, there's no need for the misc device. I do this so that in the future when someone want to implement the Ioctl(), he or she can base on this and expand it. > > > - File operation functions include: open(), read(), write() and > > flush() > > - Perform mutex lock in open() then release the mutex in flush() for > avoiding > >race condition / concurrent loading > > Make sure the mutex operation is killable, then, and maybe even > interruptable. Okay. > > > - Perform the capsule update and error return at flush() function > > > > Is there anything I missed? Any one still have concern with this idea? > > Thanks for providing the ideas as well as the review. > > > > If it works (and cat really does fail reliably), then it seems okay to me. > > However, since I like pulling increasing numbers of my hats, someone should > verify that the common embedded cat implementations are also okay with > this. For example, I haven't yet found any code in busybox's cat > implementation that closes stdout. > > Given that the main targets of this (for now, at least) are embedded, this > might be a problem. > I think we shouldn't focus on the cat implementation for the close issue. My understanding about this action: cat file.bin > /sys//capsule_loader It is actually the ">" (IO redirection) who perform the open write & close to this "/sys//capsule_loader" file note and not the "cat" do it. So, I think your answer can be found at Shell source code. More info about the IO redirection can be found at: http://www.tldp.org/LDP/abs/html/io-redirection.html Busybox Shell Souce code can be found at: https://lxr.missinglinkelectronics.com/busybox/shell/ash.c Regards, Wilson
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Thursday, April 30, 2015 2:41 AM On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: Dear communities, I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell If you do this, there's no need for the misc device. I do this so that in the future when someone want to implement the Ioctl(), he or she can base on this and expand it. - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading Make sure the mutex operation is killable, then, and maybe even interruptable. Okay. - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. If it works (and cat really does fail reliably), then it seems okay to me. However, since I like pulling increasing numbers of my hats, someone should verify that the common embedded cat implementations are also okay with this. For example, I haven't yet found any code in busybox's cat implementation that closes stdout. Given that the main targets of this (for now, at least) are embedded, this might be a problem. I think we shouldn't focus on the cat implementation for the close issue. My understanding about this action: cat file.bin /sys//capsule_loader It is actually the (IO redirection) who perform the open write close to this /sys//capsule_loader file note and not the cat do it. So, I think your answer can be found at Shell source code. More info about the IO redirection can be found at: http://www.tldp.org/LDP/abs/html/io-redirection.html Busybox Shell Souce code can be found at: https://lxr.missinglinkelectronics.com/busybox/shell/ash.c Regards, Wilson
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 30, 2015 at 2:17 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Thursday, April 30, 2015 2:41 AM On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: Dear communities, I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell If you do this, there's no need for the misc device. I do this so that in the future when someone want to implement the Ioctl(), he or she can base on this and expand it. - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading Make sure the mutex operation is killable, then, and maybe even interruptable. Okay. - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. If it works (and cat really does fail reliably), then it seems okay to me. However, since I like pulling increasing numbers of my hats, someone should verify that the common embedded cat implementations are also okay with this. For example, I haven't yet found any code in busybox's cat implementation that closes stdout. Given that the main targets of this (for now, at least) are embedded, this might be a problem. I think we shouldn't focus on the cat implementation for the close issue. My understanding about this action: cat file.bin /sys//capsule_loader It is actually the (IO redirection) who perform the open write close to this /sys//capsule_loader file note and not the cat do it. So, I think your answer can be found at Shell source code. The shell opens capsule_loader and then execs the command. If you type: (cat file.bin) /sys/.../captule_loader, then the command is a subshell and the subshell will close the file. (cat might also close it, but there will be two references.) If you type: cat file.bin /sys/.../capsule_loader then the shell doesn't retain a reference to the file at all. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 2:39 PM, James Bottomley wrote: > On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: >> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley >> wrote: >> > On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: >> >> I agree with James. Due to different people may have different needs. But >> >> from our side, we would just like to have a simple interface for us to >> >> upload >> >> the efi capsule and perform update. We do not have any use case or need >> >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: >> >> please allow me to focus on deliver this simple loading interface and >> >> upstream it. Then later whoever has the actual use case or needs on the >> >> ioctl >> >> implementation, he or she could enhance base on this simple loading >> >> interface. >> >> What do you guys think? >> >> >> >> Let me summarize the latest design idea: >> >> - No longer leverage on firmware class but use misc device >> >> - Do not use platform device but use device_create() >> >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in >> >> the shell >> >> - File operation functions include: open(), read(), write() and flush() >> >> - Perform mutex lock in open() then release the mutex in flush() for >> >> avoiding >> >>race condition / concurrent loading >> >> - Perform the capsule update and error return at flush() function >> >> >> >> Is there anything I missed? Any one still have concern with this idea? >> >> Thanks for providing the ideas as well as the review. >> > >> > I think that's pretty much it. >> > >> > Why don't you let me construct a straw man patch. It's going to be a >> > bit controversial because it involves adding flush operations to sysfs >> > and kernfs, slicing apart firmware_class.c to extract the transaction >> > handling stuff and creating an new efi update capsule file which makes >> > use of it. >> > >> > Once we have code, we at least have something more concrete to argue >> > over. >> >> Would it be worth checking whether busybox is also okay with it first? >> (Sorry to be a naysayer.) >> >> It would be a shame if we do all this to keep the userspace footprint >> light and then it doesn't work for non-coreutils userspace. > > I don't think so, because we can fix busybox if it's a problem. The > embedded people wanting this control the tool space, so they can decide > to use the fixed version. > > So yes, someone should check and fix busybox cat if broken, but no, it's > not a blocker. It's still a bit unfortunate that: #!/bin/sh cat "$1" >/sys/whatever if [ "$?" != "0" ]; then echo "It didn't work because" ... exit 1 fi echo "It worked! Go reboot if needed." exit 0 will only work sometimes. Will people really test this on their target implementation of cat? I agree that making this possible with just shell is nice, but I'm less happy about it if it'll be unreliable. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: > On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley > wrote: > > On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: > >> I agree with James. Due to different people may have different needs. But > >> from our side, we would just like to have a simple interface for us to > >> upload > >> the efi capsule and perform update. We do not have any use case or need > >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: > >> please allow me to focus on deliver this simple loading interface and > >> upstream it. Then later whoever has the actual use case or needs on the > >> ioctl > >> implementation, he or she could enhance base on this simple loading > >> interface. > >> What do you guys think? > >> > >> Let me summarize the latest design idea: > >> - No longer leverage on firmware class but use misc device > >> - Do not use platform device but use device_create() > >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in > >> the shell > >> - File operation functions include: open(), read(), write() and flush() > >> - Perform mutex lock in open() then release the mutex in flush() for > >> avoiding > >>race condition / concurrent loading > >> - Perform the capsule update and error return at flush() function > >> > >> Is there anything I missed? Any one still have concern with this idea? > >> Thanks for providing the ideas as well as the review. > > > > I think that's pretty much it. > > > > Why don't you let me construct a straw man patch. It's going to be a > > bit controversial because it involves adding flush operations to sysfs > > and kernfs, slicing apart firmware_class.c to extract the transaction > > handling stuff and creating an new efi update capsule file which makes > > use of it. > > > > Once we have code, we at least have something more concrete to argue > > over. > > Would it be worth checking whether busybox is also okay with it first? > (Sorry to be a naysayer.) > > It would be a shame if we do all this to keep the userspace footprint > light and then it doesn't work for non-coreutils userspace. I don't think so, because we can fix busybox if it's a problem. The embedded people wanting this control the tool space, so they can decide to use the fixed version. So yes, someone should check and fix busybox cat if broken, but no, it's not a blocker. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 11:40 -0700, Andy Lutomirski wrote: > If it works (and cat really does fail reliably), then it seems okay to me. > > However, since I like pulling increasing numbers of my hats, someone > should verify that the common embedded cat implementations are also > okay with this. For example, I haven't yet found any code in > busybox's cat implementation that closes stdout. > > Given that the main targets of this (for now, at least) are embedded, > this might be a problem. I think that rabbit is a bit mixie: we already demonstrated we *can* collect the error in close. It's up to the fw vendors who want to use this method to make sure they have proper tools (and if busybox cat needs fixing, to fix it before they ship). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley wrote: > On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: >> I agree with James. Due to different people may have different needs. But >> from our side, we would just like to have a simple interface for us to upload >> the efi capsule and perform update. We do not have any use case or need >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: >> please allow me to focus on deliver this simple loading interface and >> upstream it. Then later whoever has the actual use case or needs on the ioctl >> implementation, he or she could enhance base on this simple loading >> interface. >> What do you guys think? >> >> Let me summarize the latest design idea: >> - No longer leverage on firmware class but use misc device >> - Do not use platform device but use device_create() >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the >> shell >> - File operation functions include: open(), read(), write() and flush() >> - Perform mutex lock in open() then release the mutex in flush() for avoiding >>race condition / concurrent loading >> - Perform the capsule update and error return at flush() function >> >> Is there anything I missed? Any one still have concern with this idea? >> Thanks for providing the ideas as well as the review. > > I think that's pretty much it. > > Why don't you let me construct a straw man patch. It's going to be a > bit controversial because it involves adding flush operations to sysfs > and kernfs, slicing apart firmware_class.c to extract the transaction > handling stuff and creating an new efi update capsule file which makes > use of it. > > Once we have code, we at least have something more concrete to argue > over. Would it be worth checking whether busybox is also okay with it first? (Sorry to be a naysayer.) It would be a shame if we do all this to keep the userspace footprint light and then it doesn't work for non-coreutils userspace. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: > I agree with James. Due to different people may have different needs. But > from our side, we would just like to have a simple interface for us to upload > the efi capsule and perform update. We do not have any use case or need > to get info from QueryCapsuleUpdate(). Let me give a suggestion here: > please allow me to focus on deliver this simple loading interface and > upstream it. Then later whoever has the actual use case or needs on the ioctl > implementation, he or she could enhance base on this simple loading interface. > What do you guys think? > > Let me summarize the latest design idea: > - No longer leverage on firmware class but use misc device > - Do not use platform device but use device_create() > - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the > shell > - File operation functions include: open(), read(), write() and flush() > - Perform mutex lock in open() then release the mutex in flush() for avoiding >race condition / concurrent loading > - Perform the capsule update and error return at flush() function > > Is there anything I missed? Any one still have concern with this idea? > Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong wrote: >> -Original Message- >> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] >> Sent: Tuesday, April 28, 2015 6:52 AM >> >> On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: >> > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley >> > wrote: >> > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: >> > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley >> > >> wrote: >> > >> > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: >> > >> >> > -Original Message- >> > >> >> > From: James Bottomley >> > >> >> > [mailto:james.bottom...@hansenpartnership.com] >> > >> >> > Sent: Thursday, April 23, 2015 10:10 PM >> > >> >> > >> > >> >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: >> > >> >> > > > -Original Message- >> > >> >> > > > From: James Bottomley >> > >> >> > [mailto:james.bottom...@hansenpartnership.com] >> > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> > >> >> > > > >> > >> >> > > > >> > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a >> > >> >> > > > question of whether >> > >> >> > we >> > >> >> > > > can stomach the ick factor of actually initiating a >> > >> >> > > > transaction in close ... I'm >> > >> >> > still >> > >> >> > > > feeling queasy. >> > >> >> > > >> > >> >> > > The file "close" here can I understand that the file system >> > >> >> > > will call the >> > >> >> > "release" >> > >> >> > > function at the file_operations struct? >> > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153 >> > >> >> > > 8 >> > >> >> > > >> > >> >> > > So, James you are meaning that we could initiating the >> > >> >> > > update transaction inside the f_ops->release() and return >> > >> >> > > the error code if update failed in this function? >> > >> >> > >> > >> >> > Well, that's what I was thinking. However the return value of >> > >> >> > ->release doesn't get propagated in sys_close (or indeed >> > >> >> > anywhere ... no idea why it returns an int) thanks to the task >> > >> >> > work additions, so we'd actually have to use the operation >> > >> >> > whose value is propagated in sys_close() which turns out to be >> flush. >> > >> >> > >> > >> >> > James >> > >> >> > >> > >> >> >> > >> >> Okay, I think I got you. Just to double check for in case: you >> > >> >> are meaning to implement it at f_ops->flush() instead of f_ops- >> >release(). >> > >> > >> > >> > Well, what I'm saying is that the only way to propagate an error >> > >> > to close is by returning one from the flush file_operation. >> > >> > >> > >> > Let's cc fsdevel to see if they have any brighter ideas. >> > >> > >> > >> > The problem is we need to update firmware (several megabytes of >> > >> > it) via standard system tools. We're thinking cat to a device. >> > >> > The problem is that we need an error code back once the update >> > >> > goes through (which it can't until we've fed all the firmware >> > >> > data into the system). To use standard unix tools, we have to >> > >> > trigger off the standard system calls cat uses and since write() >> > >> > will happen in chunks, the only way to commit the transaction is in >> close(). >> > >> > >> > >> > We initially through of initiating the transaction in >> > >> > f_ops->release and returning the error code there, but that >> > >> > doesn't work because its value isn't actually propagated, so >> > >> > we're now thinking of initiating the transaction in f_ops->flush >> > >> > instead (this is a device, not a file, so it won't get any other >> > >> > flushers). Are there any other ways for us to propagate error on >> > >> > close? >> > >> > >> > >> >> > >> I think we may end up wanting to support both UpdateCapsule and >> > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe >> > >> we really should do a misc device + ioctl. >> > > >> > > To be honest, I hate ioctls ... especially the "have to use special >> > > tools" part. >> > > >> > > Would we ever want to support QueryCapsuleUpdate()? The return >> > > codes on error are the same as UpdateCapsule() but the query call >> > > does nothing on success (and the update call updates, obviously), so >> > > it seems a bit pointless if someone's gone to the trouble of getting >> > > a capsule ... they obviously want to apply it rather than know if it >> > > could be >> applied. >> > >> > I can imagine a UI that would try to validate a transaction consisting >> > of several of these things, tell the user whether it'll work and >> > whether a reboot is needed, and then do it. >> >> You mean for dependent capsules? That's a bit way overthinking the UEFI >> current use case (which is for firmware update). In theory, the persist >> across >> reboot flag can be used for OS persistent information (subject to someone >> actually coming up with an implementation). I'd code for the simple case: >> firmware update and let the rest take care of
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Tuesday, April 28, 2015 6:52 AM > > On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: > > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley > > wrote: > > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > > >> wrote: > > >> > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: > > >> >> > -Original Message- > > >> >> > From: James Bottomley > > >> >> > [mailto:james.bottom...@hansenpartnership.com] > > >> >> > Sent: Thursday, April 23, 2015 10:10 PM > > >> >> > > > >> >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > > >> >> > > > -Original Message- > > >> >> > > > From: James Bottomley > > >> >> > [mailto:james.bottom...@hansenpartnership.com] > > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > > >> >> > > > > > >> >> > > > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a > > >> >> > > > question of whether > > >> >> > we > > >> >> > > > can stomach the ick factor of actually initiating a > > >> >> > > > transaction in close ... I'm > > >> >> > still > > >> >> > > > feeling queasy. > > >> >> > > > > >> >> > > The file "close" here can I understand that the file system > > >> >> > > will call the > > >> >> > "release" > > >> >> > > function at the file_operations struct? > > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153 > > >> >> > > 8 > > >> >> > > > > >> >> > > So, James you are meaning that we could initiating the > > >> >> > > update transaction inside the f_ops->release() and return > > >> >> > > the error code if update failed in this function? > > >> >> > > > >> >> > Well, that's what I was thinking. However the return value of > > >> >> > ->release doesn't get propagated in sys_close (or indeed > > >> >> > anywhere ... no idea why it returns an int) thanks to the task > > >> >> > work additions, so we'd actually have to use the operation > > >> >> > whose value is propagated in sys_close() which turns out to be > flush. > > >> >> > > > >> >> > James > > >> >> > > > >> >> > > >> >> Okay, I think I got you. Just to double check for in case: you > > >> >> are meaning to implement it at f_ops->flush() instead of f_ops- > >release(). > > >> > > > >> > Well, what I'm saying is that the only way to propagate an error > > >> > to close is by returning one from the flush file_operation. > > >> > > > >> > Let's cc fsdevel to see if they have any brighter ideas. > > >> > > > >> > The problem is we need to update firmware (several megabytes of > > >> > it) via standard system tools. We're thinking cat to a device. > > >> > The problem is that we need an error code back once the update > > >> > goes through (which it can't until we've fed all the firmware > > >> > data into the system). To use standard unix tools, we have to > > >> > trigger off the standard system calls cat uses and since write() > > >> > will happen in chunks, the only way to commit the transaction is in > close(). > > >> > > > >> > We initially through of initiating the transaction in > > >> > f_ops->release and returning the error code there, but that > > >> > doesn't work because its value isn't actually propagated, so > > >> > we're now thinking of initiating the transaction in f_ops->flush > > >> > instead (this is a device, not a file, so it won't get any other > > >> > flushers). Are there any other ways for us to propagate error on > > >> > close? > > >> > > > >> > > >> I think we may end up wanting to support both UpdateCapsule and > > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe > > >> we really should do a misc device + ioctl. > > > > > > To be honest, I hate ioctls ... especially the "have to use special > > > tools" part. > > > > > > Would we ever want to support QueryCapsuleUpdate()? The return > > > codes on error are the same as UpdateCapsule() but the query call > > > does nothing on success (and the update call updates, obviously), so > > > it seems a bit pointless if someone's gone to the trouble of getting > > > a capsule ... they obviously want to apply it rather than know if it > > > could be > applied. > > > > I can imagine a UI that would try to validate a transaction consisting > > of several of these things, tell the user whether it'll work and > > whether a reboot is needed, and then do it. > > You mean for dependent capsules? That's a bit way overthinking the UEFI > current use case (which is for firmware update). In theory, the persist > across > reboot flag can be used for OS persistent information (subject to someone > actually coming up with an implementation). I'd code for the simple case: > firmware update and let the rest take care of itself if and when we have an > implementation. > > The last thing I want to see landing on the UEFI-SST is some hopelessly > complex and nasty capsule spec just
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Tuesday, April 28, 2015 6:52 AM On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L153 8 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops- release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the have to use special tools part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. You mean for dependent capsules? That's a bit way overthinking the UEFI current use case (which is for firmware update). In theory, the persist across reboot flag can be used for OS persistent information (subject to someone actually coming up with an implementation). I'd code for the simple case: firmware update and let the rest take care of itself if and when we have an implementation. The last thing I want to see landing on the UEFI-SST is some hopelessly complex and nasty capsule spec just because Linux implements it this way. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. No, we'll have read and write, so we can do that. As long as there's no transaction
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Tuesday, April 28, 2015 6:52 AM On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L153 8 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops- release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the have to use special tools part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. You mean for dependent capsules? That's a bit way overthinking the UEFI current use case (which is for firmware update). In theory, the persist across reboot flag can be used for OS persistent information (subject to someone actually coming up with an implementation). I'd code for the simple case: firmware update and let the rest take care of itself if and when we have an implementation. The last thing I want to see landing on the UEFI-SST is some hopelessly complex and nasty capsule spec just because Linux implements it this way. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. No, we'll
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 11:40 -0700, Andy Lutomirski wrote: If it works (and cat really does fail reliably), then it seems okay to me. However, since I like pulling increasing numbers of my hats, someone should verify that the common embedded cat implementations are also okay with this. For example, I haven't yet found any code in busybox's cat implementation that closes stdout. Given that the main targets of this (for now, at least) are embedded, this might be a problem. I think that rabbit is a bit mixie: we already demonstrated we *can* collect the error in close. It's up to the fw vendors who want to use this method to make sure they have proper tools (and if busybox cat needs fixing, to fix it before they ship). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. Would it be worth checking whether busybox is also okay with it first? (Sorry to be a naysayer.) It would be a shame if we do all this to keep the userspace footprint light and then it doesn't work for non-coreutils userspace. I don't think so, because we can fix busybox if it's a problem. The embedded people wanting this control the tool space, so they can decide to use the fixed version. So yes, someone should check and fix busybox cat if broken, but no, it's not a blocker. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. Would it be worth checking whether busybox is also okay with it first? (Sorry to be a naysayer.) It would be a shame if we do all this to keep the userspace footprint light and then it doesn't work for non-coreutils userspace. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 29, 2015 at 2:39 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-29 at 11:23 +, Kweh, Hock Leong wrote: I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform cat file.bin /sys/.../capsule_loader in the shell - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. Would it be worth checking whether busybox is also okay with it first? (Sorry to be a naysayer.) It would be a shame if we do all this to keep the userspace footprint light and then it doesn't work for non-coreutils userspace. I don't think so, because we can fix busybox if it's a problem. The embedded people wanting this control the tool space, so they can decide to use the fixed version. So yes, someone should check and fix busybox cat if broken, but no, it's not a blocker. It's still a bit unfortunate that: #!/bin/sh cat $1 /sys/whatever if [ $? != 0 ]; then echo It didn't work because ... exit 1 fi echo It worked! Go reboot if needed. exit 0 will only work sometimes. Will people really test this on their target implementation of cat? I agree that making this possible with just shell is nice, but I'm less happy about it if it'll be unreliable. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley > wrote: > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > >> wrote: > >> > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: > >> >> > -Original Message- > >> >> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > >> >> > Sent: Thursday, April 23, 2015 10:10 PM > >> >> > > >> >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > >> >> > > > -Original Message- > >> >> > > > From: James Bottomley > >> >> > [mailto:james.bottom...@hansenpartnership.com] > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > >> >> > > > > >> >> > > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a > >> >> > > > question of whether > >> >> > we > >> >> > > > can stomach the ick factor of actually initiating a transaction > >> >> > > > in close ... I'm > >> >> > still > >> >> > > > feeling queasy. > >> >> > > > >> >> > > The file "close" here can I understand that the file system will > >> >> > > call the > >> >> > "release" > >> >> > > function at the file_operations struct? > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > >> >> > > > >> >> > > So, James you are meaning that we could initiating the update > >> >> > > transaction > >> >> > > inside the f_ops->release() and return the error code if update > >> >> > > failed in this > >> >> > > function? > >> >> > > >> >> > Well, that's what I was thinking. However the return value of > >> >> > ->release > >> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea > >> >> > why > >> >> > it returns an int) thanks to the task work additions, so we'd actually > >> >> > have to use the operation whose value is propagated in sys_close() > >> >> > which > >> >> > turns out to be flush. > >> >> > > >> >> > James > >> >> > > >> >> > >> >> Okay, I think I got you. Just to double check for in case: you are > >> >> meaning > >> >> to implement it at f_ops->flush() instead of f_ops->release(). > >> > > >> > Well, what I'm saying is that the only way to propagate an error to > >> > close is by returning one from the flush file_operation. > >> > > >> > Let's cc fsdevel to see if they have any brighter ideas. > >> > > >> > The problem is we need to update firmware (several megabytes of it) via > >> > standard system tools. We're thinking cat to a device. The problem is > >> > that we need an error code back once the update goes through (which it > >> > can't until we've fed all the firmware data into the system). To use > >> > standard unix tools, we have to trigger off the standard system calls > >> > cat uses and since write() will happen in chunks, the only way to commit > >> > the transaction is in close(). > >> > > >> > We initially through of initiating the transaction in f_ops->release and > >> > returning the error code there, but that doesn't work because its value > >> > isn't actually propagated, so we're now thinking of initiating the > >> > transaction in f_ops->flush instead (this is a device, not a file, so it > >> > won't get any other flushers). Are there any other ways for us to > >> > propagate error on close? > >> > > >> > >> I think we may end up wanting to support both UpdateCapsule and > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe we > >> really should do a misc device + ioctl. > > > > To be honest, I hate ioctls ... especially the "have to use special > > tools" part. > > > > Would we ever want to support QueryCapsuleUpdate()? The return codes on > > error are the same as UpdateCapsule() but the query call does nothing on > > success (and the update call updates, obviously), so it seems a bit > > pointless if someone's gone to the trouble of getting a capsule ... they > > obviously want to apply it rather than know if it could be applied. > > I can imagine a UI that would try to validate a transaction consisting > of several of these things, tell the user whether it'll work and > whether a reboot is needed, and then do it. You mean for dependent capsules? That's a bit way overthinking the UEFI current use case (which is for firmware update). In theory, the persist across reboot flag can be used for OS persistent information (subject to someone actually coming up with an implementation). I'd code for the simple case: firmware update and let the rest take care of itself if and when we have an implementation. The last thing I want to see landing on the UEFI-SST is some hopelessly complex and nasty capsule spec just "because Linux implements it this way". > > Assuming we do, we could just use the same error on close mechanism, but > > use sysfs binary attributes ... or probably something new like a binary > > transaction attribute that does all the transaction on close magic for > > us. > > Yeah, but now we have both input
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley wrote: > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley >> wrote: >> > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: >> >> > -Original Message- >> >> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] >> >> > Sent: Thursday, April 23, 2015 10:10 PM >> >> > >> >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: >> >> > > > -Original Message- >> >> > > > From: James Bottomley >> >> > [mailto:james.bottom...@hansenpartnership.com] >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> >> > > > >> >> > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a question >> >> > > > of whether >> >> > we >> >> > > > can stomach the ick factor of actually initiating a transaction in >> >> > > > close ... I'm >> >> > still >> >> > > > feeling queasy. >> >> > > >> >> > > The file "close" here can I understand that the file system will call >> >> > > the >> >> > "release" >> >> > > function at the file_operations struct? >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 >> >> > > >> >> > > So, James you are meaning that we could initiating the update >> >> > > transaction >> >> > > inside the f_ops->release() and return the error code if update >> >> > > failed in this >> >> > > function? >> >> > >> >> > Well, that's what I was thinking. However the return value of ->release >> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why >> >> > it returns an int) thanks to the task work additions, so we'd actually >> >> > have to use the operation whose value is propagated in sys_close() which >> >> > turns out to be flush. >> >> > >> >> > James >> >> > >> >> >> >> Okay, I think I got you. Just to double check for in case: you are meaning >> >> to implement it at f_ops->flush() instead of f_ops->release(). >> > >> > Well, what I'm saying is that the only way to propagate an error to >> > close is by returning one from the flush file_operation. >> > >> > Let's cc fsdevel to see if they have any brighter ideas. >> > >> > The problem is we need to update firmware (several megabytes of it) via >> > standard system tools. We're thinking cat to a device. The problem is >> > that we need an error code back once the update goes through (which it >> > can't until we've fed all the firmware data into the system). To use >> > standard unix tools, we have to trigger off the standard system calls >> > cat uses and since write() will happen in chunks, the only way to commit >> > the transaction is in close(). >> > >> > We initially through of initiating the transaction in f_ops->release and >> > returning the error code there, but that doesn't work because its value >> > isn't actually propagated, so we're now thinking of initiating the >> > transaction in f_ops->flush instead (this is a device, not a file, so it >> > won't get any other flushers). Are there any other ways for us to >> > propagate error on close? >> > >> >> I think we may end up wanting to support both UpdateCapsule and >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe we >> really should do a misc device + ioctl. > > To be honest, I hate ioctls ... especially the "have to use special > tools" part. > > Would we ever want to support QueryCapsuleUpdate()? The return codes on > error are the same as UpdateCapsule() but the query call does nothing on > success (and the update call updates, obviously), so it seems a bit > pointless if someone's gone to the trouble of getting a capsule ... they > obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. > > Assuming we do, we could just use the same error on close mechanism, but > use sysfs binary attributes ... or probably something new like a binary > transaction attribute that does all the transaction on close magic for > us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. Sigh. This is all more complicated than it deserves to me. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > wrote: > > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: > >> > -Original Message- > >> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > >> > Sent: Thursday, April 23, 2015 10:10 PM > >> > > >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > >> > > > -Original Message- > >> > > > From: James Bottomley > >> > [mailto:james.bottom...@hansenpartnership.com] > >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > >> > > > > >> > > > > >> > > > Yes, I think we've all agreed we can do it ... it's now a question > >> > > > of whether > >> > we > >> > > > can stomach the ick factor of actually initiating a transaction in > >> > > > close ... I'm > >> > still > >> > > > feeling queasy. > >> > > > >> > > The file "close" here can I understand that the file system will call > >> > > the > >> > "release" > >> > > function at the file_operations struct? > >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > >> > > > >> > > So, James you are meaning that we could initiating the update > >> > > transaction > >> > > inside the f_ops->release() and return the error code if update failed > >> > > in this > >> > > function? > >> > > >> > Well, that's what I was thinking. However the return value of ->release > >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > >> > it returns an int) thanks to the task work additions, so we'd actually > >> > have to use the operation whose value is propagated in sys_close() which > >> > turns out to be flush. > >> > > >> > James > >> > > >> > >> Okay, I think I got you. Just to double check for in case: you are meaning > >> to implement it at f_ops->flush() instead of f_ops->release(). > > > > Well, what I'm saying is that the only way to propagate an error to > > close is by returning one from the flush file_operation. > > > > Let's cc fsdevel to see if they have any brighter ideas. > > > > The problem is we need to update firmware (several megabytes of it) via > > standard system tools. We're thinking cat to a device. The problem is > > that we need an error code back once the update goes through (which it > > can't until we've fed all the firmware data into the system). To use > > standard unix tools, we have to trigger off the standard system calls > > cat uses and since write() will happen in chunks, the only way to commit > > the transaction is in close(). > > > > We initially through of initiating the transaction in f_ops->release and > > returning the error code there, but that doesn't work because its value > > isn't actually propagated, so we're now thinking of initiating the > > transaction in f_ops->flush instead (this is a device, not a file, so it > > won't get any other flushers). Are there any other ways for us to > > propagate error on close? > > > > I think we may end up wanting to support both UpdateCapsule and > QueryCapsuleCapabilities, in which case this gets awkward. Maybe we > really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the "have to use special tools" part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley wrote: > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: >> > -Original Message- >> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] >> > Sent: Thursday, April 23, 2015 10:10 PM >> > >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: >> > > > -Original Message- >> > > > From: James Bottomley >> > [mailto:james.bottom...@hansenpartnership.com] >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> > > > >> > > > >> > > > Yes, I think we've all agreed we can do it ... it's now a question of >> > > > whether >> > we >> > > > can stomach the ick factor of actually initiating a transaction in >> > > > close ... I'm >> > still >> > > > feeling queasy. >> > > >> > > The file "close" here can I understand that the file system will call the >> > "release" >> > > function at the file_operations struct? >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 >> > > >> > > So, James you are meaning that we could initiating the update transaction >> > > inside the f_ops->release() and return the error code if update failed >> > > in this >> > > function? >> > >> > Well, that's what I was thinking. However the return value of ->release >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why >> > it returns an int) thanks to the task work additions, so we'd actually >> > have to use the operation whose value is propagated in sys_close() which >> > turns out to be flush. >> > >> > James >> > >> >> Okay, I think I got you. Just to double check for in case: you are meaning >> to implement it at f_ops->flush() instead of f_ops->release(). > > Well, what I'm saying is that the only way to propagate an error to > close is by returning one from the flush file_operation. > > Let's cc fsdevel to see if they have any brighter ideas. > > The problem is we need to update firmware (several megabytes of it) via > standard system tools. We're thinking cat to a device. The problem is > that we need an error code back once the update goes through (which it > can't until we've fed all the firmware data into the system). To use > standard unix tools, we have to trigger off the standard system calls > cat uses and since write() will happen in chunks, the only way to commit > the transaction is in close(). > > We initially through of initiating the transaction in f_ops->release and > returning the error code there, but that doesn't work because its value > isn't actually propagated, so we're now thinking of initiating the > transaction in f_ops->flush instead (this is a device, not a file, so it > won't get any other flushers). Are there any other ways for us to > propagate error on close? > I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops-release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops-release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the have to use special tools part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. You mean for dependent capsules? That's a bit way overthinking the UEFI current use case (which is for firmware update). In theory, the persist across reboot flag can be used for OS persistent information (subject to someone actually coming up with an implementation). I'd code for the simple case: firmware update and let the rest take care of itself if and when we have an implementation. The last thing I want to see landing on the UEFI-SST is some hopelessly complex and nasty capsule spec just because Linux implements it this way. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. No, we'll have read and write, so we can do that. As long as there's no transaction that can't complete in close or any sense of multiple transactions that aren't issued by open read/write close, we're covered. Sigh. This is all more complicated than it deserves to me. Be fair: it is a new interface and it works in
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops-release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the have to use special tools part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops-release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the have to use special tools part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. Sigh. This is all more complicated than it deserves to me. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: > > -Original Message- > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > > Sent: Thursday, April 23, 2015 10:10 PM > > > > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > > > > -Original Message- > > > > From: James Bottomley > > [mailto:james.bottom...@hansenpartnership.com] > > > > Sent: Wednesday, April 22, 2015 11:19 PM > > > > > > > > > > > > Yes, I think we've all agreed we can do it ... it's now a question of > > > > whether > > we > > > > can stomach the ick factor of actually initiating a transaction in > > > > close ... I'm > > still > > > > feeling queasy. > > > > > > The file "close" here can I understand that the file system will call the > > "release" > > > function at the file_operations struct? > > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > > > > > > So, James you are meaning that we could initiating the update transaction > > > inside the f_ops->release() and return the error code if update failed in > > > this > > > function? > > > > Well, that's what I was thinking. However the return value of ->release > > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > > it returns an int) thanks to the task work additions, so we'd actually > > have to use the operation whose value is propagated in sys_close() which > > turns out to be flush. > > > > James > > > > Okay, I think I got you. Just to double check for in case: you are meaning > to implement it at f_ops->flush() instead of f_ops->release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops->release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops->flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Thursday, April 23, 2015 10:10 PM On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops-flush() instead of f_ops-release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops-release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops-flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Thursday, April 23, 2015 10:10 PM
>
> On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote:
> > > -Original Message-
> > > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com]
> > > Sent: Wednesday, April 22, 2015 11:19 PM
> > >
> > >
> > > Yes, I think we've all agreed we can do it ... it's now a question of
> > > whether
> we
> > > can stomach the ick factor of actually initiating a transaction in close
> > > ... I'm
> still
> > > feeling queasy.
> >
> > The file "close" here can I understand that the file system will call the
> "release"
> > function at the file_operations struct?
> > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
> >
> > So, James you are meaning that we could initiating the update transaction
> > inside the f_ops->release() and return the error code if update failed in
> > this
> > function?
>
> Well, that's what I was thinking. However the return value of ->release
> doesn't get propagated in sys_close (or indeed anywhere ... no idea why
> it returns an int) thanks to the task work additions, so we'd actually
> have to use the operation whose value is propagated in sys_close() which
> turns out to be flush.
>
> James
>
Okay, I think I got you. Just to double check for in case: you are meaning
to implement it at f_ops->flush() instead of f_ops->release().
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jÇ«y�m��@A�a���
0��h���i
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 23, 2015 at 09:14:18AM -0700, James Bottomley wrote: > > > OK, so this is what I'm trying to understand. Why isn't a pipe to > > > firmware for something a "platform resource"? I think UEFI is in the > > > same class as ACPI which uses platform devices all over. > > > > And I hate the fact that ACPI did that, but that ship has sailed a long > > time ago. It "should" have been it's own bus and device type, but oh > > well. > > > > For a "simple" bus-less device, that has no platform resources needed > > (i.e from acpi or device tree), so you don't need the infrastructure > > from the platform core, just use a simple device_create() call, that's > > what it is there for. > > That's not confusing: ACPI shouldn't be a platform device, but something > that is should have a platform resource provided by ACPI (or device tree). > > So I think the problem is that the documentation is wrong? Platform > device isn't for "platform resources" like you said initially? > > Or do we have a more fundamental problem: You don't use the word > "platform" the same way we do? A "platform" to most people on this > thread is something designed to be delivered with the box that's not > amenable to user modification. That's why we think of UEFI (and ACPI) > as platform technologies: they come with the box (often they were > specially crafted for it) and we use their services to discover stuff > and correctly configure the OS. In this definition, almost everything > we do via UEFI manipulates "platform resources". Maybe we aren't using the word "platform" the same way. Platform devices were originally created to handle all of the "cruft" that a system has that used to be only at fixed locations on the CPU/chipset, and were not on any real "bus". Things like timers, keyboard controllers, and all of the odd embedded things that we just "knew" where in memory they were located. Then we got platform data structures, to help know "where" in memory devices were to be able to write to, and board files interacted with them that way. Because of this, and how they were just so easy to create, lots of developers were "lazy" and just put a platform device into their driver instead of having to hook it up to an existing system, or actually write a bus for it (I had an Intel laptop that shipped 1 year ago come with a driver that used a platform device instead of a "real" i2c device like the touchpad really was. Then came device tree, which leveraged platform devices because that's what they needed to control (replacing the board files.) Somewhere in there ACPI also decided to use platform devices and it makes sense now, as drivers just need to get a resource as to how to talk to the hardware, and it doesn't matter if it comes from ACPI or device tree. But for devices that are just "virtual", like this firmware loader, use the virtual bus, which happens automatically when you don't provide a parent to device_create(). That's what it is there for, your device doesn't have to deal with any "resources" that it needs to read from board files, device tree, or acpi, so it shouldn't be a platform device. Hope that helps explain things better. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, 2015-04-23 at 11:50 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote:
> > On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
> > > > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > > > > > -Original Message-
> > > > > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > > > > > Sent: Tuesday, April 14, 2015 10:09 PM
> > > > > > >
> > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > > > > > + */
> > > > > > > > +static void __exit efi_capsule_loader_exit(void)
> > > > > > > > +{
> > > > > > > > + platform_device_unregister(efi_capsule_pdev);
> > > > > > >
> > > > > > > This is not a platform device, don't abuse that interface please.
> > > > > > >
> > > > > > > greg k-h
> > > > > >
> > > > > > Okay, so you would recommend to use device_register() for this case?
> > > > > > Or you would think that this is more suitable to use
> > > > > > class_register()?
> > > > >
> > > > > A class isn't needed, you just want a device right? So just use a
> > > > > device, but not a platform device, as that isn't what you have here.
> > > >
> > > > Coming back to this, am I the only one confused here? What is a
> > > > 'platform device' then? Because if it doesn't fit a direct channel to
> > > > the platform firmware, which seems to be one of the definitions covered
> > > > in driver-model/platform.txt under devices with minimal infrastructure
> > > > then perhaps the documentation needs updating.
> > >
> > > I don't remember the original code here at all, sorry. I'm guessing
> > > that they were using a class, and a platform device together, which is
> > > not a good idea. Just make a "virtual" device, as you don't need/want
> > > any of the platform device infrastructure here, you just wanted a device
> > > node and/or a way to show up in sysfs somewhere.
> >
> > It was a platform device called efi_platform_loader and a single
> > attribute file in that device called capsule_load. I agree that if
> > we're going to use this for other things, we should probably have a uefi
> > directory somewhere (under firmware?) to collect everything together
> > rather than spraying random devices around.
> >
> > > If you have some kind of "platform resource", then you can be a platform
> > > device, otherwise please don't use that api just because it seems simple
> > > to use. Use the ones the driver core provides for you that really are
> > > just as simple (i.e. device_create()).
> >
> > OK, so this is what I'm trying to understand. Why isn't a pipe to
> > firmware for something a "platform resource"? I think UEFI is in the
> > same class as ACPI which uses platform devices all over.
>
> And I hate the fact that ACPI did that, but that ship has sailed a long
> time ago. It "should" have been it's own bus and device type, but oh
> well.
>
> For a "simple" bus-less device, that has no platform resources needed
> (i.e from acpi or device tree), so you don't need the infrastructure
> from the platform core, just use a simple device_create() call, that's
> what it is there for.
That's not confusing: ACPI shouldn't be a platform device, but something
that is should have a platform resource provided by ACPI (or device tree).
So I think the problem is that the documentation is wrong? Platform
device isn't for "platform resources" like you said initially?
Or do we have a more fundamental problem: You don't use the word
"platform" the same way we do? A "platform" to most people on this
thread is something designed to be delivered with the box that's not
amenable to user modification. That's why we think of UEFI (and ACPI)
as platform technologies: they come with the box (often they were
specially crafted for it) and we use their services to discover stuff
and correctly configure the OS. In this definition, almost everything
we do via UEFI manipulates "platform resources".
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > > -Original Message- > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > > Sent: Wednesday, April 22, 2015 11:19 PM > > > > > > Yes, I think we've all agreed we can do it ... it's now a question of > > whether we > > can stomach the ick factor of actually initiating a transaction in close > > ... I'm still > > feeling queasy. > > The file "close" here can I understand that the file system will call the > "release" > function at the file_operations struct? > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > > So, James you are meaning that we could initiating the update transaction > inside the f_ops->release() and return the error code if update failed in this > function? Well, that's what I was thinking. However the return value of ->release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote:
> On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
> > > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > > > > -Original Message-
> > > > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > > > > Sent: Tuesday, April 14, 2015 10:09 PM
> > > > > >
> > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > > > > + */
> > > > > > > +static void __exit efi_capsule_loader_exit(void)
> > > > > > > +{
> > > > > > > + platform_device_unregister(efi_capsule_pdev);
> > > > > >
> > > > > > This is not a platform device, don't abuse that interface please.
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Okay, so you would recommend to use device_register() for this case?
> > > > > Or you would think that this is more suitable to use class_register()?
> > > >
> > > > A class isn't needed, you just want a device right? So just use a
> > > > device, but not a platform device, as that isn't what you have here.
> > >
> > > Coming back to this, am I the only one confused here? What is a
> > > 'platform device' then? Because if it doesn't fit a direct channel to
> > > the platform firmware, which seems to be one of the definitions covered
> > > in driver-model/platform.txt under devices with minimal infrastructure
> > > then perhaps the documentation needs updating.
> >
> > I don't remember the original code here at all, sorry. I'm guessing
> > that they were using a class, and a platform device together, which is
> > not a good idea. Just make a "virtual" device, as you don't need/want
> > any of the platform device infrastructure here, you just wanted a device
> > node and/or a way to show up in sysfs somewhere.
>
> It was a platform device called efi_platform_loader and a single
> attribute file in that device called capsule_load. I agree that if
> we're going to use this for other things, we should probably have a uefi
> directory somewhere (under firmware?) to collect everything together
> rather than spraying random devices around.
>
> > If you have some kind of "platform resource", then you can be a platform
> > device, otherwise please don't use that api just because it seems simple
> > to use. Use the ones the driver core provides for you that really are
> > just as simple (i.e. device_create()).
>
> OK, so this is what I'm trying to understand. Why isn't a pipe to
> firmware for something a "platform resource"? I think UEFI is in the
> same class as ACPI which uses platform devices all over.
And I hate the fact that ACPI did that, but that ship has sailed a long
time ago. It "should" have been it's own bus and device type, but oh
well.
For a "simple" bus-less device, that has no platform resources needed
(i.e from acpi or device tree), so you don't need the infrastructure
from the platform core, just use a simple device_create() call, that's
what it is there for.
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Wednesday, April 22, 2015 11:19 PM > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > we > can stomach the ick factor of actually initiating a transaction in close ... > I'm still > feeling queasy. The file "close" here can I understand that the file system will call the "release" function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops->release() and return the error code if update failed in this function? Thanks & Regards, Wilson
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, 2015-04-23 at 11:50 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote:
On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use
class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
I don't remember the original code here at all, sorry. I'm guessing
that they were using a class, and a platform device together, which is
not a good idea. Just make a virtual device, as you don't need/want
any of the platform device infrastructure here, you just wanted a device
node and/or a way to show up in sysfs somewhere.
It was a platform device called efi_platform_loader and a single
attribute file in that device called capsule_load. I agree that if
we're going to use this for other things, we should probably have a uefi
directory somewhere (under firmware?) to collect everything together
rather than spraying random devices around.
If you have some kind of platform resource, then you can be a platform
device, otherwise please don't use that api just because it seems simple
to use. Use the ones the driver core provides for you that really are
just as simple (i.e. device_create()).
OK, so this is what I'm trying to understand. Why isn't a pipe to
firmware for something a platform resource? I think UEFI is in the
same class as ACPI which uses platform devices all over.
And I hate the fact that ACPI did that, but that ship has sailed a long
time ago. It should have been it's own bus and device type, but oh
well.
For a simple bus-less device, that has no platform resources needed
(i.e from acpi or device tree), so you don't need the infrastructure
from the platform core, just use a simple device_create() call, that's
what it is there for.
That's not confusing: ACPI shouldn't be a platform device, but something
that is should have a platform resource provided by ACPI (or device tree).
So I think the problem is that the documentation is wrong? Platform
device isn't for platform resources like you said initially?
Or do we have a more fundamental problem: You don't use the word
platform the same way we do? A platform to most people on this
thread is something designed to be delivered with the box that's not
amenable to user modification. That's why we think of UEFI (and ACPI)
as platform technologies: they come with the box (often they were
specially crafted for it) and we use their services to discover stuff
and correctly configure the OS. In this definition, almost everything
we do via UEFI manipulates platform resources.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 23, 2015 at 09:14:18AM -0700, James Bottomley wrote: OK, so this is what I'm trying to understand. Why isn't a pipe to firmware for something a platform resource? I think UEFI is in the same class as ACPI which uses platform devices all over. And I hate the fact that ACPI did that, but that ship has sailed a long time ago. It should have been it's own bus and device type, but oh well. For a simple bus-less device, that has no platform resources needed (i.e from acpi or device tree), so you don't need the infrastructure from the platform core, just use a simple device_create() call, that's what it is there for. That's not confusing: ACPI shouldn't be a platform device, but something that is should have a platform resource provided by ACPI (or device tree). So I think the problem is that the documentation is wrong? Platform device isn't for platform resources like you said initially? Or do we have a more fundamental problem: You don't use the word platform the same way we do? A platform to most people on this thread is something designed to be delivered with the box that's not amenable to user modification. That's why we think of UEFI (and ACPI) as platform technologies: they come with the box (often they were specially crafted for it) and we use their services to discover stuff and correctly configure the OS. In this definition, almost everything we do via UEFI manipulates platform resources. Maybe we aren't using the word platform the same way. Platform devices were originally created to handle all of the cruft that a system has that used to be only at fixed locations on the CPU/chipset, and were not on any real bus. Things like timers, keyboard controllers, and all of the odd embedded things that we just knew where in memory they were located. Then we got platform data structures, to help know where in memory devices were to be able to write to, and board files interacted with them that way. Because of this, and how they were just so easy to create, lots of developers were lazy and just put a platform device into their driver instead of having to hook it up to an existing system, or actually write a bus for it (I had an Intel laptop that shipped 1 year ago come with a driver that used a platform device instead of a real i2c device like the touchpad really was. Then came device tree, which leveraged platform devices because that's what they needed to control (replacing the board files.) Somewhere in there ACPI also decided to use platform devices and it makes sense now, as drivers just need to get a resource as to how to talk to the hardware, and it doesn't matter if it comes from ACPI or device tree. But for devices that are just virtual, like this firmware loader, use the virtual bus, which happens automatically when you don't provide a parent to device_create(). That's what it is there for, your device doesn't have to deal with any resources that it needs to read from board files, device tree, or acpi, so it shouldn't be a platform device. Hope that helps explain things better. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: -Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Well, that's what I was thinking. However the return value of -release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message- From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] Sent: Wednesday, April 22, 2015 11:19 PM Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. The file close here can I understand that the file system will call the release function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops-release() and return the error code if update failed in this function? Thanks Regards, Wilson
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote:
On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
I don't remember the original code here at all, sorry. I'm guessing
that they were using a class, and a platform device together, which is
not a good idea. Just make a virtual device, as you don't need/want
any of the platform device infrastructure here, you just wanted a device
node and/or a way to show up in sysfs somewhere.
It was a platform device called efi_platform_loader and a single
attribute file in that device called capsule_load. I agree that if
we're going to use this for other things, we should probably have a uefi
directory somewhere (under firmware?) to collect everything together
rather than spraying random devices around.
If you have some kind of platform resource, then you can be a platform
device, otherwise please don't use that api just because it seems simple
to use. Use the ones the driver core provides for you that really are
just as simple (i.e. device_create()).
OK, so this is what I'm trying to understand. Why isn't a pipe to
firmware for something a platform resource? I think UEFI is in the
same class as ACPI which uses platform devices all over.
And I hate the fact that ACPI did that, but that ship has sailed a long
time ago. It should have been it's own bus and device type, but oh
well.
For a simple bus-less device, that has no platform resources needed
(i.e from acpi or device tree), so you don't need the infrastructure
from the platform core, just use a simple device_create() call, that's
what it is there for.
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message-
From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
Sent: Thursday, April 23, 2015 10:10 PM
On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote:
-Original Message-
From: James Bottomley
[mailto:james.bottom...@hansenpartnership.com]
Sent: Wednesday, April 22, 2015 11:19 PM
Yes, I think we've all agreed we can do it ... it's now a question of
whether
we
can stomach the ick factor of actually initiating a transaction in close
... I'm
still
feeling queasy.
The file close here can I understand that the file system will call the
release
function at the file_operations struct?
http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
So, James you are meaning that we could initiating the update transaction
inside the f_ops-release() and return the error code if update failed in
this
function?
Well, that's what I was thinking. However the return value of -release
doesn't get propagated in sys_close (or indeed anywhere ... no idea why
it returns an int) thanks to the task work additions, so we'd actually
have to use the operation whose value is propagated in sys_close() which
turns out to be flush.
James
Okay, I think I got you. Just to double check for in case: you are meaning
to implement it at f_ops-flush() instead of f_ops-release().
Thanks Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jÇ«y�m��@A�a���
0��h���i
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 10:34 AM, James Bottomley
wrote:
> On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
>> On Apr 21, 2015 9:51 PM, "James Bottomley"
>> wrote:
>> >
>> > On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
>> > > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
>> > > wrote:
>> > > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
>> > > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
>> > > >> wrote:
>> > > >> > Andy, just on the misc device idea, what about triggering the
>> > > >> > capsule
>> > > >> > update from close()? In theory close returns an error code (not
>> > > >> > sure if
>> > > >> > most tools actually check this, though). That means we can do the
>> > > >> > write
>> > > >> > in chunks but pass it in atomically on the close and cat will work
>> > > >> > (provided it checks the return code of close).
>> > > >>
>> > > >> I thought about this but IIRC cat doesn't check the return value from
>> > > >> close.
>> > > >
>> > > > It does in my copy (coreutils-8.23) :
>> > > >
>> > > > if (!STREQ (infile, "-") && close (input_desc) < 0)
>> > > > {
>> > > > error (0, errno, "%s", infile);
>> > > > ok = false;
>> > > > }
>> > > > [...]
>> > > > if (have_read_stdin && close (STDIN_FILENO) < 0)
>> > > > error (EXIT_FAILURE, errno, _("closing standard input"));
>> > > >
>> > >
>> > > True, but it's stdout that we care about, not stdin :(
>> >
>> > Gosh you're determined to force me to wade through this source code,
>> > aren't you? That's handled in lib/closeout.c:
>> >
>> > /* Close standard output. On error, issue a diagnostic and _exit
>> >with status 'exit_failure'.
>> >
>> > ...
>> >
>> >
>> > The point is that, admittedly much to my surprise, it all looks to be
>> > handled by cat ... so we could proceed to have the transaction completed
>> > in close in a misc device (or a sysfs file).
>> >
>> > Unless there are any other rabbits you'd like to pull out of the hat?
>>
>> No, maybe it's okay, unless there's an issue where the error would
>> only be returned on the close of the last reference of the struct
>> file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar
>> until after cat exits.
>
> No, cat handles that too. It has an atexit() handler for closing
> stdout.
Indeed. Strace tells me that:
$ cat foo >bar
opens bar and then execs cat, so cat has the only reference to bar.
So no more rabbits from me :)
--Andy
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
> On Apr 21, 2015 9:51 PM, "James Bottomley"
> wrote:
> >
> > On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
> > > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
> > > wrote:
> > > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
> > > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> > > >> wrote:
> > > >> > Andy, just on the misc device idea, what about triggering the capsule
> > > >> > update from close()? In theory close returns an error code (not
> > > >> > sure if
> > > >> > most tools actually check this, though). That means we can do the
> > > >> > write
> > > >> > in chunks but pass it in atomically on the close and cat will work
> > > >> > (provided it checks the return code of close).
> > > >>
> > > >> I thought about this but IIRC cat doesn't check the return value from
> > > >> close.
> > > >
> > > > It does in my copy (coreutils-8.23) :
> > > >
> > > > if (!STREQ (infile, "-") && close (input_desc) < 0)
> > > > {
> > > > error (0, errno, "%s", infile);
> > > > ok = false;
> > > > }
> > > > [...]
> > > > if (have_read_stdin && close (STDIN_FILENO) < 0)
> > > > error (EXIT_FAILURE, errno, _("closing standard input"));
> > > >
> > >
> > > True, but it's stdout that we care about, not stdin :(
> >
> > Gosh you're determined to force me to wade through this source code,
> > aren't you? That's handled in lib/closeout.c:
> >
> > /* Close standard output. On error, issue a diagnostic and _exit
> >with status 'exit_failure'.
> >
> > ...
> >
> >
> > The point is that, admittedly much to my surprise, it all looks to be
> > handled by cat ... so we could proceed to have the transaction completed
> > in close in a misc device (or a sysfs file).
> >
> > Unless there are any other rabbits you'd like to pull out of the hat?
>
> No, maybe it's okay, unless there's an issue where the error would
> only be returned on the close of the last reference of the struct
> file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar
> until after cat exits.
No, cat handles that too. It has an atexit() handler for closing
stdout.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Apr 21, 2015 9:51 PM, "James Bottomley"
wrote:
>
> On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
> > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
> > wrote:
> > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
> > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> > >> wrote:
> > >> > Andy, just on the misc device idea, what about triggering the capsule
> > >> > update from close()? In theory close returns an error code (not sure
> > >> > if
> > >> > most tools actually check this, though). That means we can do the
> > >> > write
> > >> > in chunks but pass it in atomically on the close and cat will work
> > >> > (provided it checks the return code of close).
> > >>
> > >> I thought about this but IIRC cat doesn't check the return value from
> > >> close.
> > >
> > > It does in my copy (coreutils-8.23) :
> > >
> > > if (!STREQ (infile, "-") && close (input_desc) < 0)
> > > {
> > > error (0, errno, "%s", infile);
> > > ok = false;
> > > }
> > > [...]
> > > if (have_read_stdin && close (STDIN_FILENO) < 0)
> > > error (EXIT_FAILURE, errno, _("closing standard input"));
> > >
> >
> > True, but it's stdout that we care about, not stdin :(
>
> Gosh you're determined to force me to wade through this source code,
> aren't you? That's handled in lib/closeout.c:
>
> /* Close standard output. On error, issue a diagnostic and _exit
>with status 'exit_failure'.
>
> ...
>
>
> The point is that, admittedly much to my surprise, it all looks to be
> handled by cat ... so we could proceed to have the transaction completed
> in close in a misc device (or a sysfs file).
>
> Unless there are any other rabbits you'd like to pull out of the hat?
No, maybe it's okay, unless there's an issue where the error would
only be returned on the close of the last reference of the struct
file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar
until after cat exits.
--Andy
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
> > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > > > -Original Message-
> > > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > > > Sent: Tuesday, April 14, 2015 10:09 PM
> > > > >
> > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > > > + */
> > > > > > +static void __exit efi_capsule_loader_exit(void)
> > > > > > +{
> > > > > > + platform_device_unregister(efi_capsule_pdev);
> > > > >
> > > > > This is not a platform device, don't abuse that interface please.
> > > > >
> > > > > greg k-h
> > > >
> > > > Okay, so you would recommend to use device_register() for this case?
> > > > Or you would think that this is more suitable to use class_register()?
> > >
> > > A class isn't needed, you just want a device right? So just use a
> > > device, but not a platform device, as that isn't what you have here.
> >
> > Coming back to this, am I the only one confused here? What is a
> > 'platform device' then? Because if it doesn't fit a direct channel to
> > the platform firmware, which seems to be one of the definitions covered
> > in driver-model/platform.txt under devices with minimal infrastructure
> > then perhaps the documentation needs updating.
>
> I don't remember the original code here at all, sorry. I'm guessing
> that they were using a class, and a platform device together, which is
> not a good idea. Just make a "virtual" device, as you don't need/want
> any of the platform device infrastructure here, you just wanted a device
> node and/or a way to show up in sysfs somewhere.
It was a platform device called efi_platform_loader and a single
attribute file in that device called capsule_load. I agree that if
we're going to use this for other things, we should probably have a uefi
directory somewhere (under firmware?) to collect everything together
rather than spraying random devices around.
> If you have some kind of "platform resource", then you can be a platform
> device, otherwise please don't use that api just because it seems simple
> to use. Use the ones the driver core provides for you that really are
> just as simple (i.e. device_create()).
OK, so this is what I'm trying to understand. Why isn't a pipe to
firmware for something a "platform resource"? I think UEFI is in the
same class as ACPI which uses platform devices all over.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
> On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > > -Original Message-
> > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > > Sent: Tuesday, April 14, 2015 10:09 PM
> > > >
> > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > > + */
> > > > > +static void __exit efi_capsule_loader_exit(void)
> > > > > +{
> > > > > + platform_device_unregister(efi_capsule_pdev);
> > > >
> > > > This is not a platform device, don't abuse that interface please.
> > > >
> > > > greg k-h
> > >
> > > Okay, so you would recommend to use device_register() for this case?
> > > Or you would think that this is more suitable to use class_register()?
> >
> > A class isn't needed, you just want a device right? So just use a
> > device, but not a platform device, as that isn't what you have here.
>
> Coming back to this, am I the only one confused here? What is a
> 'platform device' then? Because if it doesn't fit a direct channel to
> the platform firmware, which seems to be one of the definitions covered
> in driver-model/platform.txt under devices with minimal infrastructure
> then perhaps the documentation needs updating.
I don't remember the original code here at all, sorry. I'm guessing
that they were using a class, and a platform device together, which is
not a good idea. Just make a "virtual" device, as you don't need/want
any of the platform device infrastructure here, you just wanted a device
node and/or a way to show up in sysfs somewhere.
If you have some kind of "platform resource", then you can be a platform
device, otherwise please don't use that api just because it seems simple
to use. Use the ones the driver core provides for you that really are
just as simple (i.e. device_create()).
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > -Original Message-
> > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > Sent: Tuesday, April 14, 2015 10:09 PM
> > >
> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > + */
> > > > +static void __exit efi_capsule_loader_exit(void)
> > > > +{
> > > > + platform_device_unregister(efi_capsule_pdev);
> > >
> > > This is not a platform device, don't abuse that interface please.
> > >
> > > greg k-h
> >
> > Okay, so you would recommend to use device_register() for this case?
> > Or you would think that this is more suitable to use class_register()?
>
> A class isn't needed, you just want a device right? So just use a
> device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> Yes, I think we've all agreed we can do it ... it's now a question of > whether we can stomach the ick factor of actually initiating a > transaction in close ... I'm still feeling queasy. NFS does transactions - including figuring out if the data will fit - on file close. It does that for real data so I'd relax. With hard disks we don't even complete a set of actions on close they just float around for a bit and get committed (but if there is a media failure you lose if you didnt commit them first via fsync etc) > The alternative might be a two file approach (either in sysfs or a mini > custom fs), one for load up data and the other for initiate transaction > with the data errors (like overflow) being returned on the load up file > and the transaction errors being returned on the write that initiates > the transaction. The two file problem then turns into the "which transaction of the two done in parallel" problem. > My architectural sense is that transaction on close, provided we can > make it a more universally accepted idea, has a lot of potential because > it's more intuitive than the two file approach. Agreed Alan -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 09:27 -0400, Peter Jones wrote:
> On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
> > On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> > wrote:
> > > Andy, just on the misc device idea, what about triggering the capsule
> > > update from close()? In theory close returns an error code (not sure if
> > > most tools actually check this, though). That means we can do the write
> > > in chunks but pass it in atomically on the close and cat will work
> > > (provided it checks the return code of close).
> >
> > I thought about this but IIRC cat doesn't check the return value from close.
>
> I checked this for the use case we'd talked about before - gnu cat
> /does/ check the error code, but it's easy to miss how, because
> coreutils code has some good ole' gnu-code complexity. It'll print the
> strerror() representation, but it always exits with 1 as the error
> code.
>
> Specifically the close on the output is handled by this:
> ---
> initialize_main (, );
> set_program_name (argv[0]);
> setlocale (LC_ALL, "");
> bindtextdomain (PACKAGE, LOCALEDIR);
> textdomain (PACKAGE);
>
> /* Arrange to close stdout if we exit via the
> case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
> Normally STDOUT_FILENO is used rather than stdout, so
> close_stdout does nothing. */
> atexit (close_stdout);
>
> /* Parse command line options. */
>
> while ((c = getopt_long (argc, argv, "benstuvAET", long_options, NULL))
> ---
>
> Which in turn does:
> ---
> void
> close_stdout (void)
> {
> if (close_stream (stdout) != 0
> && !(ignore_EPIPE && errno == EPIPE))
> {
> char const *write_error = _("write error");
> if (file_name)
> error (0, errno, "%s: %s", quotearg_colon (file_name),
>write_error);
> else
> error (0, errno, "%s", write_error);
>
> _exit (exit_failure);
> }
>
>if (close_stream (stderr) != 0)
> _exit (exit_failure);
> }
> ---
>
> exit_failure is a global from libcoreutils.a which cat never changes
> from the default, so it's always 1.
>
> (And of course error() is coreutils' own implementation rather than
> glibc's because hey maybe you're not using glibc, but still, it's
> there.)
>
> So it's /annoying/ to propagate the error from there programatically,
> but it can work.
Yes, I think we've all agreed we can do it ... it's now a question of
whether we can stomach the ick factor of actually initiating a
transaction in close ... I'm still feeling queasy.
There are quite a few of these 'transactional blob' problems where we'd
like to use a file/device approach because the data is just passed to
something but have problems because the something wants all or nothing
rather than chunks. I think all of us who work at the coal face on this
are not enthused by an ioctl solution because of the need for
non-standard tools to effect it.
The alternative might be a two file approach (either in sysfs or a mini
custom fs), one for load up data and the other for initiate transaction
with the data errors (like overflow) being returned on the load up file
and the transaction errors being returned on the write that initiates
the transaction.
My architectural sense is that transaction on close, provided we can
make it a more universally accepted idea, has a lot of potential because
it's more intuitive than the two file approach.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> wrote:
> > Andy, just on the misc device idea, what about triggering the capsule
> > update from close()? In theory close returns an error code (not sure if
> > most tools actually check this, though). That means we can do the write
> > in chunks but pass it in atomically on the close and cat will work
> > (provided it checks the return code of close).
>
> I thought about this but IIRC cat doesn't check the return value from close.
I checked this for the use case we'd talked about before - gnu cat
/does/ check the error code, but it's easy to miss how, because
coreutils code has some good ole' gnu-code complexity. It'll print the
strerror() representation, but it always exits with 1 as the error
code.
Specifically the close on the output is handled by this:
---
initialize_main (, );
set_program_name (argv[0]);
setlocale (LC_ALL, "");
bindtextdomain (PACKAGE, LOCALEDIR);
textdomain (PACKAGE);
/* Arrange to close stdout if we exit via the
case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
Normally STDOUT_FILENO is used rather than stdout, so
close_stdout does nothing. */
atexit (close_stdout);
/* Parse command line options. */
while ((c = getopt_long (argc, argv, "benstuvAET", long_options, NULL))
---
Which in turn does:
---
void
close_stdout (void)
{
if (close_stream (stdout) != 0
&& !(ignore_EPIPE && errno == EPIPE))
{
char const *write_error = _("write error");
if (file_name)
error (0, errno, "%s: %s", quotearg_colon (file_name),
write_error);
else
error (0, errno, "%s", write_error);
_exit (exit_failure);
}
if (close_stream (stderr) != 0)
_exit (exit_failure);
}
---
exit_failure is a global from libcoreutils.a which cat never changes
from the default, so it's always 1.
(And of course error() is coreutils' own implementation rather than
glibc's because hey maybe you're not using glibc, but still, it's
there.)
So it's /annoying/ to propagate the error from there programatically,
but it can work.
--
Peter
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
I don't remember the original code here at all, sorry. I'm guessing
that they were using a class, and a platform device together, which is
not a good idea. Just make a virtual device, as you don't need/want
any of the platform device infrastructure here, you just wanted a device
node and/or a way to show up in sysfs somewhere.
It was a platform device called efi_platform_loader and a single
attribute file in that device called capsule_load. I agree that if
we're going to use this for other things, we should probably have a uefi
directory somewhere (under firmware?) to collect everything together
rather than spraying random devices around.
If you have some kind of platform resource, then you can be a platform
device, otherwise please don't use that api just because it seems simple
to use. Use the ones the driver core provides for you that really are
just as simple (i.e. device_create()).
OK, so this is what I'm trying to understand. Why isn't a pipe to
firmware for something a platform resource? I think UEFI is in the
same class as ACPI which uses platform devices all over.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Apr 21, 2015 9:51 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure
if
most tools actually check this, though). That means we can do the
write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from
close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
True, but it's stdout that we care about, not stdin :(
Gosh you're determined to force me to wade through this source code,
aren't you? That's handled in lib/closeout.c:
/* Close standard output. On error, issue a diagnostic and _exit
with status 'exit_failure'.
...
The point is that, admittedly much to my surprise, it all looks to be
handled by cat ... so we could proceed to have the transaction completed
in close in a misc device (or a sysfs file).
Unless there are any other rabbits you'd like to pull out of the hat?
No, maybe it's okay, unless there's an issue where the error would
only be returned on the close of the last reference of the struct
file. After all, 'cat foo /sys/bar' doesn't fully close /sys/bar
until after cat exits.
--Andy
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure if
most tools actually check this, though). That means we can do the write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from close.
I checked this for the use case we'd talked about before - gnu cat
/does/ check the error code, but it's easy to miss how, because
coreutils code has some good ole' gnu-code complexity. It'll print the
strerror() representation, but it always exits with 1 as the error
code.
Specifically the close on the output is handled by this:
---
initialize_main (argc, argv);
set_program_name (argv[0]);
setlocale (LC_ALL, );
bindtextdomain (PACKAGE, LOCALEDIR);
textdomain (PACKAGE);
/* Arrange to close stdout if we exit via the
case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
Normally STDOUT_FILENO is used rather than stdout, so
close_stdout does nothing. */
atexit (close_stdout);
/* Parse command line options. */
while ((c = getopt_long (argc, argv, benstuvAET, long_options, NULL))
---
Which in turn does:
---
void
close_stdout (void)
{
if (close_stream (stdout) != 0
!(ignore_EPIPE errno == EPIPE))
{
char const *write_error = _(write error);
if (file_name)
error (0, errno, %s: %s, quotearg_colon (file_name),
write_error);
else
error (0, errno, %s, write_error);
_exit (exit_failure);
}
if (close_stream (stderr) != 0)
_exit (exit_failure);
}
---
exit_failure is a global from libcoreutils.a which cat never changes
from the default, so it's always 1.
(And of course error() is coreutils' own implementation rather than
glibc's because hey maybe you're not using glibc, but still, it's
there.)
So it's /annoying/ to propagate the error from there programatically,
but it can work.
--
Peter
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. NFS does transactions - including figuring out if the data will fit - on file close. It does that for real data so I'd relax. With hard disks we don't even complete a set of actions on close they just float around for a bit and get committed (but if there is a media failure you lose if you didnt commit them first via fsync etc) The alternative might be a two file approach (either in sysfs or a mini custom fs), one for load up data and the other for initiate transaction with the data errors (like overflow) being returned on the load up file and the transaction errors being returned on the write that initiates the transaction. The two file problem then turns into the which transaction of the two done in parallel problem. My architectural sense is that transaction on close, provided we can make it a more universally accepted idea, has a lot of potential because it's more intuitive than the two file approach. Agreed Alan -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
Coming back to this, am I the only one confused here? What is a
'platform device' then? Because if it doesn't fit a direct channel to
the platform firmware, which seems to be one of the definitions covered
in driver-model/platform.txt under devices with minimal infrastructure
then perhaps the documentation needs updating.
I don't remember the original code here at all, sorry. I'm guessing
that they were using a class, and a platform device together, which is
not a good idea. Just make a virtual device, as you don't need/want
any of the platform device infrastructure here, you just wanted a device
node and/or a way to show up in sysfs somewhere.
If you have some kind of platform resource, then you can be a platform
device, otherwise please don't use that api just because it seems simple
to use. Use the ones the driver core provides for you that really are
just as simple (i.e. device_create()).
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 09:27 -0400, Peter Jones wrote:
On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure if
most tools actually check this, though). That means we can do the write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from close.
I checked this for the use case we'd talked about before - gnu cat
/does/ check the error code, but it's easy to miss how, because
coreutils code has some good ole' gnu-code complexity. It'll print the
strerror() representation, but it always exits with 1 as the error
code.
Specifically the close on the output is handled by this:
---
initialize_main (argc, argv);
set_program_name (argv[0]);
setlocale (LC_ALL, );
bindtextdomain (PACKAGE, LOCALEDIR);
textdomain (PACKAGE);
/* Arrange to close stdout if we exit via the
case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
Normally STDOUT_FILENO is used rather than stdout, so
close_stdout does nothing. */
atexit (close_stdout);
/* Parse command line options. */
while ((c = getopt_long (argc, argv, benstuvAET, long_options, NULL))
---
Which in turn does:
---
void
close_stdout (void)
{
if (close_stream (stdout) != 0
!(ignore_EPIPE errno == EPIPE))
{
char const *write_error = _(write error);
if (file_name)
error (0, errno, %s: %s, quotearg_colon (file_name),
write_error);
else
error (0, errno, %s, write_error);
_exit (exit_failure);
}
if (close_stream (stderr) != 0)
_exit (exit_failure);
}
---
exit_failure is a global from libcoreutils.a which cat never changes
from the default, so it's always 1.
(And of course error() is coreutils' own implementation rather than
glibc's because hey maybe you're not using glibc, but still, it's
there.)
So it's /annoying/ to propagate the error from there programatically,
but it can work.
Yes, I think we've all agreed we can do it ... it's now a question of
whether we can stomach the ick factor of actually initiating a
transaction in close ... I'm still feeling queasy.
There are quite a few of these 'transactional blob' problems where we'd
like to use a file/device approach because the data is just passed to
something but have problems because the something wants all or nothing
rather than chunks. I think all of us who work at the coal face on this
are not enthused by an ioctl solution because of the need for
non-standard tools to effect it.
The alternative might be a two file approach (either in sysfs or a mini
custom fs), one for load up data and the other for initiate transaction
with the data errors (like overflow) being returned on the load up file
and the transaction errors being returned on the write that initiates
the transaction.
My architectural sense is that transaction on close, provided we can
make it a more universally accepted idea, has a lot of potential because
it's more intuitive than the two file approach.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
On Apr 21, 2015 9:51 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not
sure if
most tools actually check this, though). That means we can do the
write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from
close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
True, but it's stdout that we care about, not stdin :(
Gosh you're determined to force me to wade through this source code,
aren't you? That's handled in lib/closeout.c:
/* Close standard output. On error, issue a diagnostic and _exit
with status 'exit_failure'.
...
The point is that, admittedly much to my surprise, it all looks to be
handled by cat ... so we could proceed to have the transaction completed
in close in a misc device (or a sysfs file).
Unless there are any other rabbits you'd like to pull out of the hat?
No, maybe it's okay, unless there's an issue where the error would
only be returned on the close of the last reference of the struct
file. After all, 'cat foo /sys/bar' doesn't fully close /sys/bar
until after cat exits.
No, cat handles that too. It has an atexit() handler for closing
stdout.
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 22, 2015 at 10:34 AM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
On Apr 21, 2015 9:51 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the
capsule
update from close()? In theory close returns an error code (not
sure if
most tools actually check this, though). That means we can do the
write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from
close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
True, but it's stdout that we care about, not stdin :(
Gosh you're determined to force me to wade through this source code,
aren't you? That's handled in lib/closeout.c:
/* Close standard output. On error, issue a diagnostic and _exit
with status 'exit_failure'.
...
The point is that, admittedly much to my surprise, it all looks to be
handled by cat ... so we could proceed to have the transaction completed
in close in a misc device (or a sysfs file).
Unless there are any other rabbits you'd like to pull out of the hat?
No, maybe it's okay, unless there's an issue where the error would
only be returned on the close of the last reference of the struct
file. After all, 'cat foo /sys/bar' doesn't fully close /sys/bar
until after cat exits.
No, cat handles that too. It has an atexit() handler for closing
stdout.
Indeed. Strace tells me that:
$ cat foo bar
opens bar and then execs cat, so cat has the only reference to bar.
So no more rabbits from me :)
--Andy
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
> On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
> wrote:
> > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
> >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> >> wrote:
> >> > Andy, just on the misc device idea, what about triggering the capsule
> >> > update from close()? In theory close returns an error code (not sure if
> >> > most tools actually check this, though). That means we can do the write
> >> > in chunks but pass it in atomically on the close and cat will work
> >> > (provided it checks the return code of close).
> >>
> >> I thought about this but IIRC cat doesn't check the return value from
> >> close.
> >
> > It does in my copy (coreutils-8.23) :
> >
> > if (!STREQ (infile, "-") && close (input_desc) < 0)
> > {
> > error (0, errno, "%s", infile);
> > ok = false;
> > }
> > [...]
> > if (have_read_stdin && close (STDIN_FILENO) < 0)
> > error (EXIT_FAILURE, errno, _("closing standard input"));
> >
>
> True, but it's stdout that we care about, not stdin :(
Gosh you're determined to force me to wade through this source code,
aren't you? That's handled in lib/closeout.c:
/* Close standard output. On error, issue a diagnostic and _exit
with status 'exit_failure'.
...
The point is that, admittedly much to my surprise, it all looks to be
handled by cat ... so we could proceed to have the transaction completed
in close in a misc device (or a sysfs file).
Unless there are any other rabbits you'd like to pull out of the hat?
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
wrote:
> On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
>> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
>> wrote:
>> > Andy, just on the misc device idea, what about triggering the capsule
>> > update from close()? In theory close returns an error code (not sure if
>> > most tools actually check this, though). That means we can do the write
>> > in chunks but pass it in atomically on the close and cat will work
>> > (provided it checks the return code of close).
>>
>> I thought about this but IIRC cat doesn't check the return value from close.
>
> It does in my copy (coreutils-8.23) :
>
> if (!STREQ (infile, "-") && close (input_desc) < 0)
> {
> error (0, errno, "%s", infile);
> ok = false;
> }
> [...]
> if (have_read_stdin && close (STDIN_FILENO) < 0)
> error (EXIT_FAILURE, errno, _("closing standard input"));
>
True, but it's stdout that we care about, not stdin :(
--Andy
>
> James
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
> wrote:
> > Andy, just on the misc device idea, what about triggering the capsule
> > update from close()? In theory close returns an error code (not sure if
> > most tools actually check this, though). That means we can do the write
> > in chunks but pass it in atomically on the close and cat will work
> > (provided it checks the return code of close).
>
> I thought about this but IIRC cat doesn't check the return value from close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, "-") && close (input_desc) < 0)
{
error (0, errno, "%s", infile);
ok = false;
}
[...]
if (have_read_stdin && close (STDIN_FILENO) < 0)
error (EXIT_FAILURE, errno, _("closing standard input"));
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley wrote: > Andy, just on the misc device idea, what about triggering the capsule > update from close()? In theory close returns an error code (not sure if > most tools actually check this, though). That means we can do the write > in chunks but pass it in atomically on the close and cat will work > (provided it checks the return code of close). I thought about this but IIRC cat doesn't check the return value from close. --Andy > > James > > -- Andy Lutomirski AMA Capital Management, LLC -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 09:56 +0200, Greg Kroah-Hartman wrote: > On Tue, Apr 21, 2015 at 03:23:55AM +, Kweh, Hock Leong wrote: > > > -Original Message- > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > > Sent: Monday, April 20, 2015 10:43 PM > > > > > > On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: > > > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 > > > > status > > > match > > > > with the capsule upload binary? Is it okay to have one sysfs file note > > > > to tell > > > the > > > > overall status (for example: 10 capsule binaries uploaded but one > > > > require > > > > reboot, so the status shows reboot require is yes)? I am not here > > > > trying to > > > argue > > > > anything. I am just trying to find out what kind of info is needed but > > > > the > > > sysfs > > > > could not provide. > > > > > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit > > > > into > > > 6MB > > > > space and you don't even have the gcc compiler included into the > > > > package. > > > > I believe in this environment, kernel interface + shell command is the > > > > only > > > > interaction that user could work with. > > > > > > Why would you have to have gcc on such a system? Why is that a > > > requirement for having an ioctl/char interface? > > > > This is my logic: > > - Besides writing a C program (for example), I am not aware any shell script > > could perform an ioctl function call. This led me to if I don't have an > > execution > > binary then I need a compiler to compile the source to execution binary. > > You would have built it on a separate machine, like the one you used to > build your kernel and other binary packages you are running. > > > - For embedded product as mentioned above, not all vendors willing to carry > > the userland tool when they are struggling to fit into small memory space. > > Yet, you may say this tool would not eat up a lot of space compare to > > others. > > But when the source of this tool being upstream-ed to the tools/ kernel > > tree, > > we cannot stop people to contribute and make the tool more features > > support, > > eventually the embedded product may need to drop the tool. > > So because someday in the future someone might make the code that is > open source too big for us to use, we are going to reject the idea > today? That really doesn't make any sense. I think we can all agree that interfaces that don't require special purpose tools are easier to use. That doesn't mean that every interface has to not use them, because that would be impossible. However it does mean that if we can get away without using them, we should. > > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > > > no way that firmware can get that small. > > > > Actually there is. Quark is one of the examples. The kernel + rootfs take > > up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. > > If you have an Intel Galileo board, you don't need any mass storage (SD & > > USB), > > you are able to boot to Linux console. > > Does Galieleo support this UEFI feature? If so, great, how big is a > binary that can read a file and write the data using an ioctl? The capsule file is usually the same size as the NVRAM for an embedded system (on Galileo Gen I, this is 8MB) it usually includes not only the UEFI but also the OS payload. I actually load UEFI only capsules on Galileo and they're around 2MB. There have been a lot of red herrings in this thread (like namespace confusion and ideas about how big UEFI FW can be), but the problem summary is: We need a way of updating FW including payload via the UEFI capsule mechanism. Since the payload is usually as big as the NVRAM and the NVRAM contains the OS, we can't place the payload at any OS location. Unlike usual firmware, the capsule update is fire and forget (once the update is done we don't need the capsule file anymore). So what we need is a way to load the capsule data from arbitrary storage and then trigger the update. Andy, just on the misc device idea, what about triggering the capsule update from close()? In theory close returns an error code (not sure if most tools actually check this, though). That means we can do the write in chunks but pass it in atomically on the close and cat will work (provided it checks the return code of close). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 03:23:55AM +, Kweh, Hock Leong wrote: > > -Original Message- > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > Sent: Monday, April 20, 2015 10:43 PM > > > > On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: > > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 > > > status > > match > > > with the capsule upload binary? Is it okay to have one sysfs file note to > > > tell > > the > > > overall status (for example: 10 capsule binaries uploaded but one require > > > reboot, so the status shows reboot require is yes)? I am not here trying > > > to > > argue > > > anything. I am just trying to find out what kind of info is needed but the > > sysfs > > > could not provide. > > > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit > > > into > > 6MB > > > space and you don't even have the gcc compiler included into the package. > > > I believe in this environment, kernel interface + shell command is the > > > only > > > interaction that user could work with. > > > > Why would you have to have gcc on such a system? Why is that a > > requirement for having an ioctl/char interface? > > This is my logic: > - Besides writing a C program (for example), I am not aware any shell script > could perform an ioctl function call. This led me to if I don't have an > execution > binary then I need a compiler to compile the source to execution binary. You would have built it on a separate machine, like the one you used to build your kernel and other binary packages you are running. > - For embedded product as mentioned above, not all vendors willing to carry > the userland tool when they are struggling to fit into small memory space. > Yet, you may say this tool would not eat up a lot of space compare to > others. > But when the source of this tool being upstream-ed to the tools/ kernel > tree, > we cannot stop people to contribute and make the tool more features support, > eventually the embedded product may need to drop the tool. So because someday in the future someone might make the code that is open source too big for us to use, we are going to reject the idea today? That really doesn't make any sense. > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > > no way that firmware can get that small. > > Actually there is. Quark is one of the examples. The kernel + rootfs take > up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. > If you have an Intel Galileo board, you don't need any mass storage (SD & > USB), > you are able to boot to Linux console. Does Galieleo support this UEFI feature? If so, great, how big is a binary that can read a file and write the data using an ioctl? 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 03:23:55AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Monday, April 20, 2015 10:43 PM On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. You would have built it on a separate machine, like the one you used to build your kernel and other binary packages you are running. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. So because someday in the future someone might make the code that is open source too big for us to use, we are going to reject the idea today? That really doesn't make any sense. And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD USB), you are able to boot to Linux console. Does Galieleo support this UEFI feature? If so, great, how big is a binary that can read a file and write the data using an ioctl? 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 09:56 +0200, Greg Kroah-Hartman wrote: On Tue, Apr 21, 2015 at 03:23:55AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Monday, April 20, 2015 10:43 PM On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. You would have built it on a separate machine, like the one you used to build your kernel and other binary packages you are running. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. So because someday in the future someone might make the code that is open source too big for us to use, we are going to reject the idea today? That really doesn't make any sense. I think we can all agree that interfaces that don't require special purpose tools are easier to use. That doesn't mean that every interface has to not use them, because that would be impossible. However it does mean that if we can get away without using them, we should. And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD USB), you are able to boot to Linux console. Does Galieleo support this UEFI feature? If so, great, how big is a binary that can read a file and write the data using an ioctl? The capsule file is usually the same size as the NVRAM for an embedded system (on Galileo Gen I, this is 8MB) it usually includes not only the UEFI but also the OS payload. I actually load UEFI only capsules on Galileo and they're around 2MB. There have been a lot of red herrings in this thread (like namespace confusion and ideas about how big UEFI FW can be), but the problem summary is: We need a way of updating FW including payload via the UEFI capsule mechanism. Since the payload is usually as big as the NVRAM and the NVRAM contains the OS, we can't place the payload at any OS location. Unlike usual firmware, the capsule update is fire and forget (once the update is done we don't need the capsule file anymore). So what we need is a way to load the capsule data from arbitrary storage and then trigger the update. Andy, just on the misc device idea, what about triggering the capsule update from close()? In theory close returns an error code (not sure if most tools actually check this, though). That means we can do the write in chunks but pass it in atomically on the close and cat will work (provided it checks the return code of close). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: Andy, just on the misc device idea, what about triggering the capsule update from close()? In theory close returns an error code (not sure if most tools actually check this, though). That means we can do the write in chunks but pass it in atomically on the close and cat will work (provided it checks the return code of close). I thought about this but IIRC cat doesn't check the return value from close. --Andy James -- Andy Lutomirski AMA Capital Management, LLC -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure if
most tools actually check this, though). That means we can do the write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure if
most tools actually check this, though). That means we can do the write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
True, but it's stdout that we care about, not stdin :(
--Andy
James
--
Andy Lutomirski
AMA Capital Management, LLC
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote:
On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
Andy, just on the misc device idea, what about triggering the capsule
update from close()? In theory close returns an error code (not sure if
most tools actually check this, though). That means we can do the write
in chunks but pass it in atomically on the close and cat will work
(provided it checks the return code of close).
I thought about this but IIRC cat doesn't check the return value from
close.
It does in my copy (coreutils-8.23) :
if (!STREQ (infile, -) close (input_desc) 0)
{
error (0, errno, %s, infile);
ok = false;
}
[...]
if (have_read_stdin close (STDIN_FILENO) 0)
error (EXIT_FAILURE, errno, _(closing standard input));
True, but it's stdout that we care about, not stdin :(
Gosh you're determined to force me to wade through this source code,
aren't you? That's handled in lib/closeout.c:
/* Close standard output. On error, issue a diagnostic and _exit
with status 'exit_failure'.
...
The point is that, admittedly much to my surprise, it all looks to be
handled by cat ... so we could proceed to have the transaction completed
in close in a misc device (or a sysfs file).
Unless there are any other rabbits you'd like to pull out of the hat?
James
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > Sent: Monday, April 20, 2015 10:43 PM > > On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 > > status > match > > with the capsule upload binary? Is it okay to have one sysfs file note to > > tell > the > > overall status (for example: 10 capsule binaries uploaded but one require > > reboot, so the status shows reboot require is yes)? I am not here trying to > argue > > anything. I am just trying to find out what kind of info is needed but the > sysfs > > could not provide. > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > 6MB > > space and you don't even have the gcc compiler included into the package. > > I believe in this environment, kernel interface + shell command is the only > > interaction that user could work with. > > Why would you have to have gcc on such a system? Why is that a > requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD & USB), you are able to boot to Linux console. Thanks & Regards, Wilson -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 2015-04-17 at 15:49 +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 16, 2015 at 09:42:31AM +, Kweh, Hock Leong wrote: > > > -Original Message- > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > > Sent: Wednesday, April 15, 2015 9:19 PM > > > > > > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote: > > > > > -Original Message- > > > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > > From: "Kweh, Hock Leong" > > > > > > > > > > > > Introducing a kernel module to expose capsule loader interface > > > > > > for user to upload capsule binaries. This module leverage the > > > > > > request_firmware_direct_full_path() to obtain the binary at a > > > > > > specific path input by user. > > > > > > > > > > > > Example method to load the capsule binary: > > > > > > echo -n "/path/to/capsule/binary" > > > > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > > > > > Ick, why not just have the firmware file location present, and copy it > > > > > to the sysfs file directly from userspace, instead of this two-step > > > > > process? > > > > > > > > Err I may not catch your meaning correctly. Are you trying to say > > > > that you would prefer the user to perform: > > > > > > > > cat file.bin > /sys/.../capsule_loader > > > > > > > > instead of > > > > > > > > echo -n "/path/to/binary" > /sys//capsule_laoder > > > > > > Yes. What's the namespace of your /path/to/binary/ and how do you know > > > the kernel has the same one when it does the firmware load call? By > > > just copying the data with 'cat', you don't have to worry about > > > namespace issues at all. > > > > Hi Greg, > > > > Let me double confirm that I understand your concern correctly. You are > > trying to tell that some others module may use a 'same' namespace to > > request the firmware but never release it. Then when our module trying > > to request the firmware by passing in the 'same' namespace, I will get the > > previous data instead of the current binary data from the path I want. > > Yes. > > > Hmm I believe this concern also apply to all the current > > request_firmware > > APIs right? And I believe the coincidence to have 'same' file name namespace > > would be higher than full path namespace. > > Not really, the kernel namespace is what matters at that point in time. > > And maybe it does matter, I haven't thought through all of the issues. > But passing a path from userspace, to the kernel, to have the kernel > turn around again and use that path is full of nasty consequences at > times due to namespaces, let's avoid all of that please. So just to clarify this, namespaces are designed not to cause a problem here, provided the operation is handled correctly (this is key; it is easy do design operations which will screw up no end if done wrongly). The file name to object translation is handled by the mount name space, which is the operative one of the process doing the echo. For a longstanding object (i.e. one which will exist beyond the call to the system of the current process) you need either to convert to the actual underlying object (usually a file descriptor) which has an existence independent of the namespace (and perform all the necessary security validations before returning control back to userspace, so they occur within all the namespace constraints of the calling process), or store sufficient information to redo whatever operation you need to within the namespace (the former is by far preferred for long lived operations). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: > Regarding the 'reboot require' status, is it critical to have a 1 to 1 status > match > with the capsule upload binary? Is it okay to have one sysfs file note to > tell the > overall status (for example: 10 capsule binaries uploaded but one require > reboot, so the status shows reboot require is yes)? I am not here trying to > argue > anything. I am just trying to find out what kind of info is needed but the > sysfs > could not provide. > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > 6MB > space and you don't even have the gcc compiler included into the package. > I believe in this environment, kernel interface + shell command is the only > interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. > Btw, just to make sure I get it correctly, is misc device refer to the device > that created by misc_register() from drivers/char/misc.c and not asked to > put this kernel module under drivers/misc/* location, right? Yes, use misc_register() > And Matt mentioned including the source into tools/* in kernel tree. I have > a question: Is this tool can be compiled during kernel compilation and > eventually auto included into the rootfs package? Sorry, I am new to OS > creation and maybe this is stupid question. If you ask to build it as part of the configuration, yes, it can be built. See how the tools are build as part of the kernel tree for more information about this. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Monday, April 20, 2015 10:43 PM On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD USB), you are able to boot to Linux console. Thanks Regards, Wilson -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Mon, Apr 20, 2015 at 03:28:32AM +, Kweh, Hock Leong wrote: Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. Btw, just to make sure I get it correctly, is misc device refer to the device that created by misc_register() from drivers/char/misc.c and not asked to put this kernel module under drivers/misc/* location, right? Yes, use misc_register() And Matt mentioned including the source into tools/* in kernel tree. I have a question: Is this tool can be compiled during kernel compilation and eventually auto included into the rootfs package? Sorry, I am new to OS creation and maybe this is stupid question. If you ask to build it as part of the configuration, yes, it can be built. See how the tools are build as part of the kernel tree for more information about this. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 2015-04-17 at 15:49 +0200, Greg Kroah-Hartman wrote: On Thu, Apr 16, 2015 at 09:42:31AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Wednesday, April 15, 2015 9:19 PM On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Tuesday, April 14, 2015 10:09 PM On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Err I may not catch your meaning correctly. Are you trying to say that you would prefer the user to perform: cat file.bin /sys/.../capsule_loader instead of echo -n /path/to/binary /sys//capsule_laoder Yes. What's the namespace of your /path/to/binary/ and how do you know the kernel has the same one when it does the firmware load call? By just copying the data with 'cat', you don't have to worry about namespace issues at all. Hi Greg, Let me double confirm that I understand your concern correctly. You are trying to tell that some others module may use a 'same' namespace to request the firmware but never release it. Then when our module trying to request the firmware by passing in the 'same' namespace, I will get the previous data instead of the current binary data from the path I want. Yes. Hmm I believe this concern also apply to all the current request_firmware APIs right? And I believe the coincidence to have 'same' file name namespace would be higher than full path namespace. Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that please. So just to clarify this, namespaces are designed not to cause a problem here, provided the operation is handled correctly (this is key; it is easy do design operations which will screw up no end if done wrongly). The file name to object translation is handled by the mount name space, which is the operative one of the process doing the echo. For a longstanding object (i.e. one which will exist beyond the call to the system of the current process) you need either to convert to the actual underlying object (usually a file descriptor) which has an existence independent of the namespace (and perform all the necessary security validations before returning control back to userspace, so they occur within all the namespace constraints of the calling process), or store sufficient information to redo whatever operation you need to within the namespace (the former is by far preferred for long lived operations). James -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Matt Fleming [mailto:m...@codeblueprint.co.uk] > Sent: Friday, April 17, 2015 10:37 PM > > On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: > > > > Not really, the kernel namespace is what matters at that point in time. > > > > And maybe it does matter, I haven't thought through all of the issues. > > But passing a path from userspace, to the kernel, to have the kernel > > turn around again and use that path is full of nasty consequences at > > times due to namespaces, let's avoid all of that please. > > Oh crap. The namespace issue is a good point and not something I'd > thought of at all. > > At this point, I think we've really run out of objections to Andy's > suggestion of implementing this as a misc device, right? > > The concern I had about userspace tooling can partly be addressed by > including the source in tools/ in the kernel tree. So provided we do > that, I'm happy enough to see this implemented as a misc device because > the other options we've explored just haven't panned out. > > Objections? > > -- > Matt Fleming, Intel Open Source Technology Center Hi Greg, Matt & Andy, Wait wait a minute. I am lost. I think I have missed something. Let me summarize the message I got from the email threads. = Greg:- Recommends to use "cat file.bin > /sys/.../capsule_loader" due to there is concern on kernel namespace with this submitted design which using the request firmware API. Andy:- Prefer to have an interface that could return the error code and suggested char device where the ioctl can meet the purpose. Matt:- Prefer to use kernel interface only as embedded world may not want to include userland tool. == I think I have missed somewhere why not using "cat file.bin > /sys/.../loader" and now change to misc device. Is it because the ioctl could return a custom error code (for example: platform not supported, capsule header error) where the "cat file.bin > /sys/.../loader" interface cannot do? Hmm .. Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Btw, just to make sure I get it correctly, is misc device refer to the device that created by misc_register() from drivers/char/misc.c and not asked to put this kernel module under drivers/misc/* location, right? And Matt mentioned including the source into tools/* in kernel tree. I have a question: Is this tool can be compiled during kernel compilation and eventually auto included into the rootfs package? Sorry, I am new to OS creation and maybe this is stupid question. Thanks & Regards, Wilson -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message- From: Matt Fleming [mailto:m...@codeblueprint.co.uk] Sent: Friday, April 17, 2015 10:37 PM On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that please. Oh crap. The namespace issue is a good point and not something I'd thought of at all. At this point, I think we've really run out of objections to Andy's suggestion of implementing this as a misc device, right? The concern I had about userspace tooling can partly be addressed by including the source in tools/ in the kernel tree. So provided we do that, I'm happy enough to see this implemented as a misc device because the other options we've explored just haven't panned out. Objections? -- Matt Fleming, Intel Open Source Technology Center Hi Greg, Matt Andy, Wait wait a minute. I am lost. I think I have missed something. Let me summarize the message I got from the email threads. = Greg:- Recommends to use cat file.bin /sys/.../capsule_loader due to there is concern on kernel namespace with this submitted design which using the request firmware API. Andy:- Prefer to have an interface that could return the error code and suggested char device where the ioctl can meet the purpose. Matt:- Prefer to use kernel interface only as embedded world may not want to include userland tool. == I think I have missed somewhere why not using cat file.bin /sys/.../loader and now change to misc device. Is it because the ioctl could return a custom error code (for example: platform not supported, capsule header error) where the cat file.bin /sys/.../loader interface cannot do? Hmm .. Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Btw, just to make sure I get it correctly, is misc device refer to the device that created by misc_register() from drivers/char/misc.c and not asked to put this kernel module under drivers/misc/* location, right? And Matt mentioned including the source into tools/* in kernel tree. I have a question: Is this tool can be compiled during kernel compilation and eventually auto included into the rootfs package? Sorry, I am new to OS creation and maybe this is stupid question. Thanks Regards, Wilson -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: > > Not really, the kernel namespace is what matters at that point in time. > > And maybe it does matter, I haven't thought through all of the issues. > But passing a path from userspace, to the kernel, to have the kernel > turn around again and use that path is full of nasty consequences at > times due to namespaces, let's avoid all of that please. Oh crap. The namespace issue is a good point and not something I'd thought of at all. At this point, I think we've really run out of objections to Andy's suggestion of implementing this as a misc device, right? The concern I had about userspace tooling can partly be addressed by including the source in tools/ in the kernel tree. So provided we do that, I'm happy enough to see this implemented as a misc device because the other options we've explored just haven't panned out. Objections? -- Matt Fleming, Intel Open Source Technology Center -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 05:19:09PM -0700, Roy Franz wrote: > On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski wrote: > > On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" > > wrote: > >> > >> On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > >> > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > >> > wrote: > >> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > >> > >> From: "Kweh, Hock Leong" > >> > >> > >> > >> Introducing a kernel module to expose capsule loader interface > >> > >> for user to upload capsule binaries. This module leverage the > >> > >> request_firmware_direct_full_path() to obtain the binary at a > >> > >> specific path input by user. > >> > >> > >> > >> Example method to load the capsule binary: > >> > >> echo -n "/path/to/capsule/binary" > > >> > >> /sys/devices/platform/efi_capsule_loader/capsule_loader > >> > > > >> > > Ick, why not just have the firmware file location present, and copy it > >> > > to the sysfs file directly from userspace, instead of this two-step > >> > > process? > >> > > >> > Because it's not at all obvious how error handling should work in that > >> > case. > >> > >> I don't understand how the error handling is any different. The kernel > >> ends up copying the data in through the firmware interface both ways, we > >> just aren't creating yet-another-firmware-path in the system. > > > > If I run uefi-update-capsule foo.bin, I want it to complain if the > > UEFI call fails. In contrast, if I boot and my ath10k firmware is > > bad, there's no explicit user interaction that should fail; instead I > > have no network device and I get to read the logs and figure out why. > > IOW bad volatile device firmware is just like a bad device driver, > > whereas bad UEFI capsules are problems that should be reported to > > whatever tried to send them to UEFI. > > > > --Andy > > > There are several types of errors that can be returned by > UpdateCapsule(), allowing > us to distinguish between capsules that are not supported by the > platform, capsules > that must be updated at boot time, and capsule updates that failed due > to a device error. > I think we really do want this type of information returned to capsule loader. Ok, all of that sounds like you really want a character device, with an ioctl, to do this. Just use a misc device and your infrastructure will be handled for you (sysfs, character device, etc.) and away you go. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 16, 2015 at 09:42:31AM +, Kweh, Hock Leong wrote: > > -Original Message- > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > Sent: Wednesday, April 15, 2015 9:19 PM > > > > On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote: > > > > -Original Message- > > > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > From: "Kweh, Hock Leong" > > > > > > > > > > Introducing a kernel module to expose capsule loader interface > > > > > for user to upload capsule binaries. This module leverage the > > > > > request_firmware_direct_full_path() to obtain the binary at a > > > > > specific path input by user. > > > > > > > > > > Example method to load the capsule binary: > > > > > echo -n "/path/to/capsule/binary" > > > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > > > Ick, why not just have the firmware file location present, and copy it > > > > to the sysfs file directly from userspace, instead of this two-step > > > > process? > > > > > > Err I may not catch your meaning correctly. Are you trying to say > > > that you would prefer the user to perform: > > > > > > cat file.bin > /sys/.../capsule_loader > > > > > > instead of > > > > > > echo -n "/path/to/binary" > /sys//capsule_laoder > > > > Yes. What's the namespace of your /path/to/binary/ and how do you know > > the kernel has the same one when it does the firmware load call? By > > just copying the data with 'cat', you don't have to worry about > > namespace issues at all. > > Hi Greg, > > Let me double confirm that I understand your concern correctly. You are > trying to tell that some others module may use a 'same' namespace to > request the firmware but never release it. Then when our module trying > to request the firmware by passing in the 'same' namespace, I will get the > previous data instead of the current binary data from the path I want. Yes. > Hmm I believe this concern also apply to all the current request_firmware > APIs right? And I believe the coincidence to have 'same' file name namespace > would be higher than full path namespace. Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that please. Oh crap. The namespace issue is a good point and not something I'd thought of at all. At this point, I think we've really run out of objections to Andy's suggestion of implementing this as a misc device, right? The concern I had about userspace tooling can partly be addressed by including the source in tools/ in the kernel tree. So provided we do that, I'm happy enough to see this implemented as a misc device because the other options we've explored just haven't panned out. Objections? -- Matt Fleming, Intel Open Source Technology Center -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 05:19:09PM -0700, Roy Franz wrote: On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski l...@amacapital.net wrote: On Apr 15, 2015 6:20 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy There are several types of errors that can be returned by UpdateCapsule(), allowing us to distinguish between capsules that are not supported by the platform, capsules that must be updated at boot time, and capsule updates that failed due to a device error. I think we really do want this type of information returned to capsule loader. Ok, all of that sounds like you really want a character device, with an ioctl, to do this. Just use a misc device and your infrastructure will be handled for you (sysfs, character device, etc.) and away you go. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Thu, Apr 16, 2015 at 09:42:31AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Wednesday, April 15, 2015 9:19 PM On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote: -Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Tuesday, April 14, 2015 10:09 PM On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Err I may not catch your meaning correctly. Are you trying to say that you would prefer the user to perform: cat file.bin /sys/.../capsule_loader instead of echo -n /path/to/binary /sys//capsule_laoder Yes. What's the namespace of your /path/to/binary/ and how do you know the kernel has the same one when it does the firmware load call? By just copying the data with 'cat', you don't have to worry about namespace issues at all. Hi Greg, Let me double confirm that I understand your concern correctly. You are trying to tell that some others module may use a 'same' namespace to request the firmware but never release it. Then when our module trying to request the firmware by passing in the 'same' namespace, I will get the previous data instead of the current binary data from the path I want. Yes. Hmm I believe this concern also apply to all the current request_firmware APIs right? And I believe the coincidence to have 'same' file name namespace would be higher than full path namespace. Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, April 15, 2015 9:19 PM
>
> On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > > -Original Message-
> > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > Sent: Tuesday, April 14, 2015 10:09 PM
> > >
> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > From: "Kweh, Hock Leong"
> > > >
> > > > Introducing a kernel module to expose capsule loader interface
> > > > for user to upload capsule binaries. This module leverage the
> > > > request_firmware_direct_full_path() to obtain the binary at a
> > > > specific path input by user.
> > > >
> > > > Example method to load the capsule binary:
> > > > echo -n "/path/to/capsule/binary" >
> > > /sys/devices/platform/efi_capsule_loader/capsule_loader
> > >
> > > Ick, why not just have the firmware file location present, and copy it
> > > to the sysfs file directly from userspace, instead of this two-step
> > > process?
> >
> > Err I may not catch your meaning correctly. Are you trying to say
> > that you would prefer the user to perform:
> >
> > cat file.bin > /sys/.../capsule_loader
> >
> > instead of
> >
> > echo -n "/path/to/binary" > /sys//capsule_laoder
>
> Yes. What's the namespace of your /path/to/binary/ and how do you know
> the kernel has the same one when it does the firmware load call? By
> just copying the data with 'cat', you don't have to worry about
> namespace issues at all.
Hi Greg,
Let me double confirm that I understand your concern correctly. You are
trying to tell that some others module may use a 'same' namespace to
request the firmware but never release it. Then when our module trying
to request the firmware by passing in the 'same' namespace, I will get the
previous data instead of the current binary data from the path I want.
Hmm I believe this concern also apply to all the current request_firmware
APIs right? And I believe the coincidence to have 'same' file name namespace
would be higher than full path namespace.
I may not able to control other module developers on the namespace naming.
But I could ensure each firmware request will be released before the next
request firmware happen in this module. This module will return error if it
does not receive a real efi capsule binary.
Hmm ..
>
> > The reason we stick with the firmware_class is because we don't
> > want to replicate a code which already mature and has open API
> > for developer to use.
>
> That's fine, but adding a new api to the firmware interface seems odd to
> me, just because you don't like using /lib/ or any of the other
> "standard" locations for firmware blobs. And note, that path is
> configurable.
If I am not mistaken, I believe you are referring the module param path.
Yes, I do aware of it. If I need a more flexibility method to alter the custom
path, the current design would require system reboot or module re-insmod.
So, leveraging the request_firmware_direct_full_path() could make things
a little bit easier from this point of view.
Besides, this new API actually helps to overcome the confusedness of having
2 or more same file name binaries at the firmware search paths (fw_path_para;
/lib/firmware/update/; /lib/firmware/). Due to user have the possibility to
configure kernel command param / module param for this fw_path_para,
it may have chances to point to a path that have same file name the
/lib/firmware/
also have. With this API, it can make it specific to the path that developer
wants.
>
> > > > + */
> > > > +static void __exit efi_capsule_loader_exit(void)
> > > > +{
> > > > + platform_device_unregister(efi_capsule_pdev);
> > >
> > > This is not a platform device, don't abuse that interface please.
> > >
> > > greg k-h
> >
> > Okay, so you would recommend to use device_register() for this case?
> > Or you would think that this is more suitable to use class_register()?
>
> A class isn't needed, you just want a device right? So just use a
> device, but not a platform device, as that isn't what you have here.
>
> thanks,
>
> greg k-h
Okay, will do this. Thanks.
Regards,
Wilson
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Wednesday, April 15, 2015 9:19 PM
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n /path/to/capsule/binary
/sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin /sys/.../capsule_loader
instead of
echo -n /path/to/binary /sys//capsule_laoder
Yes. What's the namespace of your /path/to/binary/ and how do you know
the kernel has the same one when it does the firmware load call? By
just copying the data with 'cat', you don't have to worry about
namespace issues at all.
Hi Greg,
Let me double confirm that I understand your concern correctly. You are
trying to tell that some others module may use a 'same' namespace to
request the firmware but never release it. Then when our module trying
to request the firmware by passing in the 'same' namespace, I will get the
previous data instead of the current binary data from the path I want.
Hmm I believe this concern also apply to all the current request_firmware
APIs right? And I believe the coincidence to have 'same' file name namespace
would be higher than full path namespace.
I may not able to control other module developers on the namespace naming.
But I could ensure each firmware request will be released before the next
request firmware happen in this module. This module will return error if it
does not receive a real efi capsule binary.
Hmm ..
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
That's fine, but adding a new api to the firmware interface seems odd to
me, just because you don't like using /lib/ or any of the other
standard locations for firmware blobs. And note, that path is
configurable.
If I am not mistaken, I believe you are referring the module param path.
Yes, I do aware of it. If I need a more flexibility method to alter the custom
path, the current design would require system reboot or module re-insmod.
So, leveraging the request_firmware_direct_full_path() could make things
a little bit easier from this point of view.
Besides, this new API actually helps to overcome the confusedness of having
2 or more same file name binaries at the firmware search paths (fw_path_para;
/lib/firmware/update/; /lib/firmware/). Due to user have the possibility to
configure kernel command param / module param for this fw_path_para,
it may have chances to point to a path that have same file name the
/lib/firmware/
also have. With this API, it can make it specific to the path that developer
wants.
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
thanks,
greg k-h
Okay, will do this. Thanks.
Regards,
Wilson
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski wrote: > On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" > wrote: >> >> On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: >> > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman >> > wrote: >> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: >> > >> From: "Kweh, Hock Leong" >> > >> >> > >> Introducing a kernel module to expose capsule loader interface >> > >> for user to upload capsule binaries. This module leverage the >> > >> request_firmware_direct_full_path() to obtain the binary at a >> > >> specific path input by user. >> > >> >> > >> Example method to load the capsule binary: >> > >> echo -n "/path/to/capsule/binary" > >> > >> /sys/devices/platform/efi_capsule_loader/capsule_loader >> > > >> > > Ick, why not just have the firmware file location present, and copy it >> > > to the sysfs file directly from userspace, instead of this two-step >> > > process? >> > >> > Because it's not at all obvious how error handling should work in that >> > case. >> >> I don't understand how the error handling is any different. The kernel >> ends up copying the data in through the firmware interface both ways, we >> just aren't creating yet-another-firmware-path in the system. > > If I run uefi-update-capsule foo.bin, I want it to complain if the > UEFI call fails. In contrast, if I boot and my ath10k firmware is > bad, there's no explicit user interaction that should fail; instead I > have no network device and I get to read the logs and figure out why. > IOW bad volatile device firmware is just like a bad device driver, > whereas bad UEFI capsules are problems that should be reported to > whatever tried to send them to UEFI. > > --Andy > There are several types of errors that can be returned by UpdateCapsule(), allowing us to distinguish between capsules that are not supported by the platform, capsules that must be updated at boot time, and capsule updates that failed due to a device error. I think we really do want this type of information returned to capsule loader. Roy >> >> thanks, >> >> greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" wrote: > > On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > > wrote: > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > >> From: "Kweh, Hock Leong" > > >> > > >> Introducing a kernel module to expose capsule loader interface > > >> for user to upload capsule binaries. This module leverage the > > >> request_firmware_direct_full_path() to obtain the binary at a > > >> specific path input by user. > > >> > > >> Example method to load the capsule binary: > > >> echo -n "/path/to/capsule/binary" > > > >> /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > Ick, why not just have the firmware file location present, and copy it > > > to the sysfs file directly from userspace, instead of this two-step > > > process? > > > > Because it's not at all obvious how error handling should work in that case. > > I don't understand how the error handling is any different. The kernel > ends up copying the data in through the firmware interface both ways, we > just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy > > 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > wrote: > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > >> From: "Kweh, Hock Leong" > >> > >> Introducing a kernel module to expose capsule loader interface > >> for user to upload capsule binaries. This module leverage the > >> request_firmware_direct_full_path() to obtain the binary at a > >> specific path input by user. > >> > >> Example method to load the capsule binary: > >> echo -n "/path/to/capsule/binary" > > >> /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > Ick, why not just have the firmware file location present, and copy it > > to the sysfs file directly from userspace, instead of this two-step > > process? > > Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
> > -Original Message-
> > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > Sent: Tuesday, April 14, 2015 10:09 PM
> >
> > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > From: "Kweh, Hock Leong"
> > >
> > > Introducing a kernel module to expose capsule loader interface
> > > for user to upload capsule binaries. This module leverage the
> > > request_firmware_direct_full_path() to obtain the binary at a
> > > specific path input by user.
> > >
> > > Example method to load the capsule binary:
> > > echo -n "/path/to/capsule/binary" >
> > /sys/devices/platform/efi_capsule_loader/capsule_loader
> >
> > Ick, why not just have the firmware file location present, and copy it
> > to the sysfs file directly from userspace, instead of this two-step
> > process?
>
> Err I may not catch your meaning correctly. Are you trying to say
> that you would prefer the user to perform:
>
> cat file.bin > /sys/.../capsule_loader
>
> instead of
>
> echo -n "/path/to/binary" > /sys//capsule_laoder
Yes. What's the namespace of your /path/to/binary/ and how do you know
the kernel has the same one when it does the firmware load call? By
just copying the data with 'cat', you don't have to worry about
namespace issues at all.
> The reason we stick with the firmware_class is because we don't
> want to replicate a code which already mature and has open API
> for developer to use.
That's fine, but adding a new api to the firmware interface seems odd to
me, just because you don't like using /lib/ or any of the other
"standard" locations for firmware blobs. And note, that path is
configurable.
> > > + */
> > > +static void __exit efi_capsule_loader_exit(void)
> > > +{
> > > + platform_device_unregister(efi_capsule_pdev);
> >
> > This is not a platform device, don't abuse that interface please.
> >
> > greg k-h
>
> Okay, so you would recommend to use device_register() for this case?
> Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, April 14, 2015 10:09 PM
>
> On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong"
> >
> > Introducing a kernel module to expose capsule loader interface
> > for user to upload capsule binaries. This module leverage the
> > request_firmware_direct_full_path() to obtain the binary at a
> > specific path input by user.
> >
> > Example method to load the capsule binary:
> > echo -n "/path/to/capsule/binary" >
> /sys/devices/platform/efi_capsule_loader/capsule_loader
>
> Ick, why not just have the firmware file location present, and copy it
> to the sysfs file directly from userspace, instead of this two-step
> process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin > /sys/.../capsule_loader
instead of
echo -n "/path/to/binary" > /sys//capsule_laoder
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
>
> > +/*
> > + * To remove this kernel module, just perform:
> > + * rmmod efi_capsule_loader.ko
>
> This comment is not needed.
Okay, I will remove this.
>
>
> > + */
> > +static void __exit efi_capsule_loader_exit(void)
> > +{
> > + platform_device_unregister(efi_capsule_pdev);
>
> This is not a platform device, don't abuse that interface please.
>
> greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
Thanks & Regards,
Wilson
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 11:32:29AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n /path/to/capsule/binary
/sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin /sys/.../capsule_loader
instead of
echo -n /path/to/binary /sys//capsule_laoder
Yes. What's the namespace of your /path/to/binary/ and how do you know
the kernel has the same one when it does the firmware load call? By
just copying the data with 'cat', you don't have to worry about
namespace issues at all.
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
That's fine, but adding a new api to the firmware interface seems odd to
me, just because you don't like using /lib/ or any of the other
standard locations for firmware blobs. And note, that path is
configurable.
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
A class isn't needed, you just want a device right? So just use a
device, but not a platform device, as that isn't what you have here.
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Apr 15, 2015 6:20 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski l...@amacapital.net wrote: On Apr 15, 2015 6:20 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy There are several types of errors that can be returned by UpdateCapsule(), allowing us to distinguish between capsules that are not supported by the platform, capsules that must be updated at boot time, and capsule updates that failed due to a device error. I think we really do want this type of information returned to capsule loader. Roy thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.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 v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n /path/to/capsule/binary
/sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin /sys/.../capsule_loader
instead of
echo -n /path/to/binary /sys//capsule_laoder
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
+/*
+ * To remove this kernel module, just perform:
+ * rmmod efi_capsule_loader.ko
This comment is not needed.
Okay, I will remove this.
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
Thanks Regards,
Wilson
--
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman wrote: > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: >> From: "Kweh, Hock Leong" >> >> Introducing a kernel module to expose capsule loader interface >> for user to upload capsule binaries. This module leverage the >> request_firmware_direct_full_path() to obtain the binary at a >> specific path input by user. >> >> Example method to load the capsule binary: >> echo -n "/path/to/capsule/binary" > >> /sys/devices/platform/efi_capsule_loader/capsule_loader > > Ick, why not just have the firmware file location present, and copy it > to the sysfs file directly from userspace, instead of this two-step > process? Because it's not at all obvious how error handling should work in that case. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong"
>
> Introducing a kernel module to expose capsule loader interface
> for user to upload capsule binaries. This module leverage the
> request_firmware_direct_full_path() to obtain the binary at a
> specific path input by user.
>
> Example method to load the capsule binary:
> echo -n "/path/to/capsule/binary" >
> /sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
> +/*
> + * To remove this kernel module, just perform:
> + * rmmod efi_capsule_loader.ko
This comment is not needed.
> + */
> +static void __exit efi_capsule_loader_exit(void)
> +{
> + platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. --Andy -- 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 v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n /path/to/capsule/binary
/sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
+/*
+ * To remove this kernel module, just perform:
+ * rmmod efi_capsule_loader.ko
This comment is not needed.
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
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/
