Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 26, 2017 at 07:28:12PM -0700, Vikram Mulukutla wrote: > On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote: > > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > > > Was it used because your version has taken so long to be > > > submitted/reviwed? > > > > Vikram would have a better idea as he is the one who authored it, but it > > would seem this effort was in parallel to my own at that time. > > > > I must shamefully admit that the story is a bit older - the patch I > originally worked on was on a v3.4 based tree. Oh wow so we had *two* separate parallel efforts to simplify this code somehow... My earliest sysdata API was based on v4.2-rc5 [0], this was after we decided we *wanted* to enable to pass more arguments for fw signing from the start, to enable custom fw criteria, as my original fw signing effort was completely transparent to the API and matched what we did with module signing [1], and based on v4.1-rc3. Only difference is you just worked on the internal data tossed around. I provided a way to also use this for growing the API. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20150805-sysdata [1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=fw-signing-v2-20150513 > We had been forward > porting it until Stephen Boyd was kind enough (or tired of it) to take > time out of his clock maintainer-ship and upstream the > request_firmware_into_buf API. At that point of time it seemed that the > 'desc' approach was unnecessary, and I agreed. It was very much needed and it could have helped. Next time please just send patches right away! > So Luis's series came > in much later and wasn't a factor in forward-porting the patches. > While it does seem that the _internal_ implementation of > firmware_class can be a bit friendlier to adding the features that > are on their way, I can't say the same about the API being exposed to > drivers in mainline; maintainers and folks with more experience in > kernel API evolution are better equipped to answer that question. I actually am not aware how seriously the postulation to the problem I decided to take on is being considered here... Some of it may seem straight forward to some based on experience, but due to the size of the kernel inspired by my prior effort to study collateral evolutions for both forward and backporting purposes, I've decided to take on the problem in a bit different light. Just as your primary reason for your changes was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable", I also believed that the way in which we were loosely growing the firmware API through unnecessary collateral evolutions was untenable. I will confess that growing the API was just one consideration, another long term lofty goal also aims towards automatic test driver generation, and enough is sprinkled on test_driver_data.c that I hope some could infer perhaps how I started thinking that might be possible one day. I'm happy to park such effort, so long as we just decide with a path forward so we can move on and chug on. Perhaps in the future some folks may want to re-evaluate and consider the approach a bit further. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 26, 2017 at 07:28:12PM -0700, Vikram Mulukutla wrote: > On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote: > > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > > > Was it used because your version has taken so long to be > > > submitted/reviwed? > > > > Vikram would have a better idea as he is the one who authored it, but it > > would seem this effort was in parallel to my own at that time. > > > > I must shamefully admit that the story is a bit older - the patch I > originally worked on was on a v3.4 based tree. Oh wow so we had *two* separate parallel efforts to simplify this code somehow... My earliest sysdata API was based on v4.2-rc5 [0], this was after we decided we *wanted* to enable to pass more arguments for fw signing from the start, to enable custom fw criteria, as my original fw signing effort was completely transparent to the API and matched what we did with module signing [1], and based on v4.1-rc3. Only difference is you just worked on the internal data tossed around. I provided a way to also use this for growing the API. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20150805-sysdata [1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=fw-signing-v2-20150513 > We had been forward > porting it until Stephen Boyd was kind enough (or tired of it) to take > time out of his clock maintainer-ship and upstream the > request_firmware_into_buf API. At that point of time it seemed that the > 'desc' approach was unnecessary, and I agreed. It was very much needed and it could have helped. Next time please just send patches right away! > So Luis's series came > in much later and wasn't a factor in forward-porting the patches. > While it does seem that the _internal_ implementation of > firmware_class can be a bit friendlier to adding the features that > are on their way, I can't say the same about the API being exposed to > drivers in mainline; maintainers and folks with more experience in > kernel API evolution are better equipped to answer that question. I actually am not aware how seriously the postulation to the problem I decided to take on is being considered here... Some of it may seem straight forward to some based on experience, but due to the size of the kernel inspired by my prior effort to study collateral evolutions for both forward and backporting purposes, I've decided to take on the problem in a bit different light. Just as your primary reason for your changes was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable", I also believed that the way in which we were loosely growing the firmware API through unnecessary collateral evolutions was untenable. I will confess that growing the API was just one consideration, another long term lofty goal also aims towards automatic test driver generation, and enough is sprinkled on test_driver_data.c that I hope some could infer perhaps how I started thinking that might be possible one day. I'm happy to park such effort, so long as we just decide with a path forward so we can move on and chug on. Perhaps in the future some folks may want to re-evaluate and consider the approach a bit further. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote: On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguezwrote: Ah, yes! Here is what I believe seems to be the *crux* issue of these patch series and I'm happy we have finally landed on it. Yes, indeed the new API proposed here provides more flexibility, and it does so by embracing a "data driven" API Vs the traditional procedural APIs we have seen for *the firmware API*. This has been going on forever. Everybody hates your data-driven one. Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. Things like that may be ok as an internal implementation, but even there it's questionable if it then means a big disconnect between what people actually use (the normal functional model) and the implementation. A vendor tree implemented their *own* solution and were willing to maintain it despite this likely making it hard to port stable fixes. That I think says a lot for a need... What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. I must shamefully admit that the story is a bit older - the patch I originally worked on was on a v3.4 based tree. We had been forward porting it until Stephen Boyd was kind enough (or tired of it) to take time out of his clock maintainer-ship and upstream the request_firmware_into_buf API. At that point of time it seemed that the 'desc' approach was unnecessary, and I agreed. So Luis's series came in much later and wasn't a factor in forward-porting the patches. While it does seem that the _internal_ implementation of firmware_class can be a bit friendlier to adding the features that are on their way, I can't say the same about the API being exposed to drivers in mainline; maintainers and folks with more experience in kernel API evolution are better equipped to answer that question. Thanks, Vikram
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote: On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez wrote: Ah, yes! Here is what I believe seems to be the *crux* issue of these patch series and I'm happy we have finally landed on it. Yes, indeed the new API proposed here provides more flexibility, and it does so by embracing a "data driven" API Vs the traditional procedural APIs we have seen for *the firmware API*. This has been going on forever. Everybody hates your data-driven one. Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. Things like that may be ok as an internal implementation, but even there it's questionable if it then means a big disconnect between what people actually use (the normal functional model) and the implementation. A vendor tree implemented their *own* solution and were willing to maintain it despite this likely making it hard to port stable fixes. That I think says a lot for a need... What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. I must shamefully admit that the story is a bit older - the patch I originally worked on was on a v3.4 based tree. We had been forward porting it until Stephen Boyd was kind enough (or tired of it) to take time out of his clock maintainer-ship and upstream the request_firmware_into_buf API. At that point of time it seemed that the 'desc' approach was unnecessary, and I agreed. So Luis's series came in much later and wasn't a factor in forward-porting the patches. While it does seem that the _internal_ implementation of firmware_class can be a bit friendlier to adding the features that are on their way, I can't say the same about the API being exposed to drivers in mainline; maintainers and folks with more experience in kernel API evolution are better equipped to answer that question. Thanks, Vikram
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 26, 2017 at 08:19:07PM +0200, Rafał Miłecki wrote: > On 2017-06-26 19:33, Luis R. Rodriguez wrote: > > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > > > > There are still other requirements and features in the pipeline for > > > > which we > > > > can consider parameters to parse for, rather than adding new API. Case > > > > in > > > > point, do we want *one* API just to disable the firmware cache? > > > > Specially > > > > knowing that another feature in the pipeline later would make use of > > > > this as a > > > > requirement? > > > > > > Again, I do not care! You can not justify patches today with some > > > mythical thing in the future that might never even happen. > > > > Granting the option to make async firmware optional was discussed since > > December 2016 by RafaÅ [1]. It was only later during my driver data API > > changes that Hans noted the nvram part was actually *not* optional [2] so > > this requirement dropped. *However* as the maintainer I believ ethis > > requirement *is sensible* and would not be surprised if alternative > > firmware already exists where this is what is intended. > > I believe there was a misunderstanding of my patch by Hans. The point of my > patch was to don't display warning *IF* we can use alternative soruce and > get the NVRAM (firmware) from platform data (special partition used by the > bootloader and accessible by the operating system). Oh, are you saying the optional async firmware loading is still a requirement for this driver? Are you, Hans, and Arend Van Spriel in agreement on this? If so then that definitely makes 3 effective changes in my radar for extensions to the firmware API. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 26, 2017 at 08:19:07PM +0200, Rafał Miłecki wrote: > On 2017-06-26 19:33, Luis R. Rodriguez wrote: > > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > > > > There are still other requirements and features in the pipeline for > > > > which we > > > > can consider parameters to parse for, rather than adding new API. Case > > > > in > > > > point, do we want *one* API just to disable the firmware cache? > > > > Specially > > > > knowing that another feature in the pipeline later would make use of > > > > this as a > > > > requirement? > > > > > > Again, I do not care! You can not justify patches today with some > > > mythical thing in the future that might never even happen. > > > > Granting the option to make async firmware optional was discussed since > > December 2016 by RafaÅ [1]. It was only later during my driver data API > > changes that Hans noted the nvram part was actually *not* optional [2] so > > this requirement dropped. *However* as the maintainer I believ ethis > > requirement *is sensible* and would not be surprised if alternative > > firmware already exists where this is what is intended. > > I believe there was a misunderstanding of my patch by Hans. The point of my > patch was to don't display warning *IF* we can use alternative soruce and > get the NVRAM (firmware) from platform data (special partition used by the > bootloader and accessible by the operating system). Oh, are you saying the optional async firmware loading is still a requirement for this driver? Are you, Hans, and Arend Van Spriel in agreement on this? If so then that definitely makes 3 effective changes in my radar for extensions to the firmware API. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 2017-06-26 19:33, Luis R. Rodriguez wrote: On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguezwrote: > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > > proposed here provides more flexibility, and it does so by embracing a > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > *the firmware API*. > > > > This has been going on forever. Everybody hates your data-driven one. > > Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. > > Things like that may be ok as an internal implementation, but even > > there it's questionable if it then means a big disconnect between what > > people actually use (the normal functional model) and the > > implementation. > > A vendor tree implemented their *own* solution and were willing to maintain > it despite this likely making it hard to port stable fixes. That I think says > a lot for a need... What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. > There are still other requirements and features in the pipeline for which we > can consider parameters to parse for, rather than adding new API. Case in > point, do we want *one* API just to disable the firmware cache? Specially > knowing that another feature in the pipeline later would make use of this as a > requirement? Again, I do not care! You can not justify patches today with some mythical thing in the future that might never even happen. Some of these features are things actually being discussed for a while, so to say they are mythical is not accurate. I can trace back firmware signing discussions back to 2015, along with Plumbers in person discussions where we seem to have agreed upon a path forward among a few folks who disagreed on a technical basis. Linaro has a clear interest so AKASHI picked up that work now as I have been busy with general maintainer duties. The fact that Linus just suggested an alternative approach to a params approach is new, and yet to be reviewe by AKASHI for firmware signing. Granting the option to make async firmware optional was discussed since December 2016 by RafaÅ [1]. It was only later during my driver data API changes that Hans noted the nvram part was actually *not* optional [2] so this requirement dropped. *However* as the maintainer I believ ethis requirement *is sensible* and would not be surprised if alternative firmware already exists where this is what is intended. I believe there was a misunderstanding of my patch by Hans. The point of my patch was
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 2017-06-26 19:33, Luis R. Rodriguez wrote: On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez wrote: > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > > proposed here provides more flexibility, and it does so by embracing a > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > *the firmware API*. > > > > This has been going on forever. Everybody hates your data-driven one. > > Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. > > Things like that may be ok as an internal implementation, but even > > there it's questionable if it then means a big disconnect between what > > people actually use (the normal functional model) and the > > implementation. > > A vendor tree implemented their *own* solution and were willing to maintain > it despite this likely making it hard to port stable fixes. That I think says > a lot for a need... What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. > There are still other requirements and features in the pipeline for which we > can consider parameters to parse for, rather than adding new API. Case in > point, do we want *one* API just to disable the firmware cache? Specially > knowing that another feature in the pipeline later would make use of this as a > requirement? Again, I do not care! You can not justify patches today with some mythical thing in the future that might never even happen. Some of these features are things actually being discussed for a while, so to say they are mythical is not accurate. I can trace back firmware signing discussions back to 2015, along with Plumbers in person discussions where we seem to have agreed upon a path forward among a few folks who disagreed on a technical basis. Linaro has a clear interest so AKASHI picked up that work now as I have been busy with general maintainer duties. The fact that Linus just suggested an alternative approach to a params approach is new, and yet to be reviewe by AKASHI for firmware signing. Granting the option to make async firmware optional was discussed since December 2016 by RafaÅ [1]. It was only later during my driver data API changes that Hans noted the nvram part was actually *not* optional [2] so this requirement dropped. *However* as the maintainer I believ ethis requirement *is sensible* and would not be surprised if alternative firmware already exists where this is what is intended. I believe there was a misunderstanding of my patch by Hans. The point of my patch was to don't display
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez> > > wrote: > > > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these > > > > patch > > > > series and I'm happy we have finally landed on it. Yes, indeed the new > > > > API > > > > proposed here provides more flexibility, and it does so by embracing a > > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > > *the firmware API*. > > > > > > This has been going on forever. Everybody hates your data-driven one. > > > > Before you, the only person who had expressed disdain here was Greg. > > Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. > > > Things like that may be ok as an internal implementation, but even > > > there it's questionable if it then means a big disconnect between what > > > people actually use (the normal functional model) and the > > > implementation. > > > > A vendor tree implemented their *own* solution and were willing to maintain > > it despite this likely making it hard to port stable fixes. That I think > > says > > a lot for a need... > > What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. > Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. > > There are still other requirements and features in the pipeline for which we > > can consider parameters to parse for, rather than adding new API. Case in > > point, do we want *one* API just to disable the firmware cache? Specially > > knowing that another feature in the pipeline later would make use of this > > as a > > requirement? > > Again, I do not care! You can not justify patches today with some > mythical thing in the future that might never even happen. Some of these features are things actually being discussed for a while, so to say they are mythical is not accurate. I can trace back firmware signing discussions back to 2015, along with Plumbers in person discussions where we seem to have agreed upon a path forward among a few folks who disagreed on a technical basis. Linaro has a clear interest so AKASHI picked up that work now as I have been busy with general maintainer duties. The fact that Linus just suggested an alternative approach to a params approach is new, and yet to be reviewe by AKASHI for firmware signing. Granting the option to make async firmware optional was discussed since December 2016 by RafaÅ [1]. It was only later during my driver data API changes that Hans noted the nvram part was actually *not* optional [2] so this requirement dropped. *However* as the maintainer I believ ethis requirement *is sensible* and would not be surprised if alternative firmware already exists where this is what is intended. The streaming support for FPGAs has been going through a round of reviews
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez > > > wrote: > > > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these > > > > patch > > > > series and I'm happy we have finally landed on it. Yes, indeed the new > > > > API > > > > proposed here provides more flexibility, and it does so by embracing a > > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > > *the firmware API*. > > > > > > This has been going on forever. Everybody hates your data-driven one. > > > > Before you, the only person who had expressed disdain here was Greg. > > Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. > > > Things like that may be ok as an internal implementation, but even > > > there it's questionable if it then means a big disconnect between what > > > people actually use (the normal functional model) and the > > > implementation. > > > > A vendor tree implemented their *own* solution and were willing to maintain > > it despite this likely making it hard to port stable fixes. That I think > > says > > a lot for a need... > > What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5c...@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. > Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. > > There are still other requirements and features in the pipeline for which we > > can consider parameters to parse for, rather than adding new API. Case in > > point, do we want *one* API just to disable the firmware cache? Specially > > knowing that another feature in the pipeline later would make use of this > > as a > > requirement? > > Again, I do not care! You can not justify patches today with some > mythical thing in the future that might never even happen. Some of these features are things actually being discussed for a while, so to say they are mythical is not accurate. I can trace back firmware signing discussions back to 2015, along with Plumbers in person discussions where we seem to have agreed upon a path forward among a few folks who disagreed on a technical basis. Linaro has a clear interest so AKASHI picked up that work now as I have been busy with general maintainer duties. The fact that Linus just suggested an alternative approach to a params approach is new, and yet to be reviewe by AKASHI for firmware signing. Granting the option to make async firmware optional was discussed since December 2016 by RafaÅ [1]. It was only later during my driver data API changes that Hans noted the nvram part was actually *not* optional [2] so this requirement dropped. *However* as the maintainer I believ ethis requirement *is sensible* and would not be surprised if alternative firmware already exists where this is what is intended. The streaming support for FPGAs has been going through a round of reviews since March [3]. The
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:40:57PM +0200, Greg KH wrote: > On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote: > > > It's also blocking real bug fixes and features that people want > > > addressed, which isn't acceptable. > > > > What? What bugs ? I have addressed *every single* stable issue in queue and > > have sent patches for them and they do not depend in any way on any of this > > series! > > Ok, so we are all caught up now with bug fixes? That's good, I was not > aware of that, so nevermind that objection, my fault. Yes. I've been following up on all actively reported bugs and have even implemented better alternatives based on discussions. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:40:57PM +0200, Greg KH wrote: > On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote: > > > It's also blocking real bug fixes and features that people want > > > addressed, which isn't acceptable. > > > > What? What bugs ? I have addressed *every single* stable issue in queue and > > have sent patches for them and they do not depend in any way on any of this > > series! > > Ok, so we are all caught up now with bug fixes? That's good, I was not > aware of that, so nevermind that objection, my fault. Yes. I've been following up on all actively reported bugs and have even implemented better alternatives based on discussions. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote: > > It's also blocking real bug fixes and features that people want > > addressed, which isn't acceptable. > > What? What bugs ? I have addressed *every single* stable issue in queue and > have sent patches for them and they do not depend in any way on any of this > series! Ok, so we are all caught up now with bug fixes? That's good, I was not aware of that, so nevermind that objection, my fault. greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote: > > It's also blocking real bug fixes and features that people want > > addressed, which isn't acceptable. > > What? What bugs ? I have addressed *every single* stable issue in queue and > have sent patches for them and they do not depend in any way on any of this > series! Ok, so we are all caught up now with bug fixes? That's good, I was not aware of that, so nevermind that objection, my fault. greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez> > wrote: > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these > > > patch > > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > > proposed here provides more flexibility, and it does so by embracing a > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > *the firmware API*. > > > > This has been going on forever. Everybody hates your data-driven one. > > Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. > > Things like that may be ok as an internal implementation, but even > > there it's questionable if it then means a big disconnect between what > > people actually use (the normal functional model) and the > > implementation. > > A vendor tree implemented their *own* solution and were willing to maintain > it despite this likely making it hard to port stable fixes. That I think says > a lot for a need... What vendor tree? Where was it shipped? Why was it external and how is it different from your patches? Was it used because your version has taken so long to be submitted/reviwed? > There are still other requirements and features in the pipeline for which we > can consider parameters to parse for, rather than adding new API. Case in > point, do we want *one* API just to disable the firmware cache? Specially > knowing that another feature in the pipeline later would make use of this as a > requirement? Again, I do not care! You can not justify patches today with some mythical thing in the future that might never even happen. Again, as it stands, this patch series is unacceptable, and the added complexity of a crazy api that goes against almost all normal in-kernel apis, is only one part of the reason. The other being the loads of added code for no apparent benifit at all. So please both fix the api to be "normal", and show as to why these patches are actually needed _today_, otherwise we can just live with what we have now just fine and muddle along like always. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez > > wrote: > > > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these > > > patch > > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > > proposed here provides more flexibility, and it does so by embracing a > > > "data driven" API Vs the traditional procedural APIs we have seen for > > > *the firmware API*. > > > > This has been going on forever. Everybody hates your data-driven one. > > Before you, the only person who had expressed disdain here was Greg. Very few people actually review code, you know that. > > Things like that may be ok as an internal implementation, but even > > there it's questionable if it then means a big disconnect between what > > people actually use (the normal functional model) and the > > implementation. > > A vendor tree implemented their *own* solution and were willing to maintain > it despite this likely making it hard to port stable fixes. That I think says > a lot for a need... What vendor tree? Where was it shipped? Why was it external and how is it different from your patches? Was it used because your version has taken so long to be submitted/reviwed? > There are still other requirements and features in the pipeline for which we > can consider parameters to parse for, rather than adding new API. Case in > point, do we want *one* API just to disable the firmware cache? Specially > knowing that another feature in the pipeline later would make use of this as a > requirement? Again, I do not care! You can not justify patches today with some mythical thing in the future that might never even happen. Again, as it stands, this patch series is unacceptable, and the added complexity of a crazy api that goes against almost all normal in-kernel apis, is only one part of the reason. The other being the loads of added code for no apparent benifit at all. So please both fix the api to be "normal", and show as to why these patches are actually needed _today_, otherwise we can just live with what we have now just fine and muddle along like always. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguezwrote: > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > proposed here provides more flexibility, and it does so by embracing a > > "data driven" API Vs the traditional procedural APIs we have seen for > > *the firmware API*. > > This has been going on forever. Everybody hates your data-driven one. Before you, the only person who had expressed disdain here was Greg. > It's too indirect, it adds all those nasty "descriptors" of what to > do, and it doesn't match what the current model does at all. This is good feedback, I do accept deciding where to draw the line is hard. I decided to go with blocking/non-blocking as the fine line. > Things like that may be ok as an internal implementation, but even > there it's questionable if it then means a big disconnect between what > people actually use (the normal functional model) and the > implementation. A vendor tree implemented their *own* solution and were willing to maintain it despite this likely making it hard to port stable fixes. That I think says a lot for a need... > The thing is, it's much better to just have functions that load the > firmware data. Have them named that way ("load_firmware()"), and act > that way ("just load the damn firmware file") instead of having odd > descriptors that describe what is goign to be done and some state for > it, and then get passed around. > > Don't add this kind of crazy abstraction complexity. > > If somebody wants to veryify a signature on a firmware file, they > should *NOT* fill in a descriptor that says "check signature when > loading". Thats' complete BS. > > They should just do "load_firmware()" and then "check_signature()" or > whatever. That's a fair suggestion for firmware signing! And I'll let AKASHI comment on whether or not that would suffice for his requirements given he's now addressing firmware signing. There are still other requirements and features in the pipeline for which we can consider parameters to parse for, rather than adding new API. Case in point, do we want *one* API just to disable the firmware cache? Specially knowing that another feature in the pipeline later would make use of this as a requirement? Or let us just consider the very simple *optional* async firmware. Do we add *one* full new API call just for that? Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez wrote: > > > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > > series and I'm happy we have finally landed on it. Yes, indeed the new API > > proposed here provides more flexibility, and it does so by embracing a > > "data driven" API Vs the traditional procedural APIs we have seen for > > *the firmware API*. > > This has been going on forever. Everybody hates your data-driven one. Before you, the only person who had expressed disdain here was Greg. > It's too indirect, it adds all those nasty "descriptors" of what to > do, and it doesn't match what the current model does at all. This is good feedback, I do accept deciding where to draw the line is hard. I decided to go with blocking/non-blocking as the fine line. > Things like that may be ok as an internal implementation, but even > there it's questionable if it then means a big disconnect between what > people actually use (the normal functional model) and the > implementation. A vendor tree implemented their *own* solution and were willing to maintain it despite this likely making it hard to port stable fixes. That I think says a lot for a need... > The thing is, it's much better to just have functions that load the > firmware data. Have them named that way ("load_firmware()"), and act > that way ("just load the damn firmware file") instead of having odd > descriptors that describe what is goign to be done and some state for > it, and then get passed around. > > Don't add this kind of crazy abstraction complexity. > > If somebody wants to veryify a signature on a firmware file, they > should *NOT* fill in a descriptor that says "check signature when > loading". Thats' complete BS. > > They should just do "load_firmware()" and then "check_signature()" or > whatever. That's a fair suggestion for firmware signing! And I'll let AKASHI comment on whether or not that would suffice for his requirements given he's now addressing firmware signing. There are still other requirements and features in the pipeline for which we can consider parameters to parse for, rather than adding new API. Case in point, do we want *one* API just to disable the firmware cache? Specially knowing that another feature in the pipeline later would make use of this as a requirement? Or let us just consider the very simple *optional* async firmware. Do we add *one* full new API call just for that? Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguezwrote: > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > series and I'm happy we have finally landed on it. Yes, indeed the new API > proposed here provides more flexibility, and it does so by embracing a > "data driven" API Vs the traditional procedural APIs we have seen for > *the firmware API*. This has been going on forever. Everybody hates your data-driven one. It's too indirect, it adds all those nasty "descriptors" of what to do, and it doesn't match what the current model does at all. Things like that may be ok as an internal implementation, but even there it's questionable if it then means a big disconnect between what people actually use (the normal functional model) and the implementation. The thing is, it's much better to just have functions that load the firmware data. Have them named that way ("load_firmware()"), and act that way ("just load the damn firmware file") instead of having odd descriptors that describe what is goign to be done and some state for it, and then get passed around. Don't add this kind of crazy abstraction complexity. If somebody wants to veryify a signature on a firmware file, they should *NOT* fill in a descriptor that says "check signature when loading". Thats' complete BS. They should just do "load_firmware()" and then "check_signature()" or whatever. Would such a "load firmware file, then check signature" take a few lines (with error handling)? Yes. But it is a *simple* interface. It doesn't have some stupid "struct driver_data_params" that needs to be filled in with random details (or has magic macros in a header file that fill in default values). It's _straightforward_. Linus
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez wrote: > > Ah, yes! Here is what I believe seems to be the *crux* issue of these patch > series and I'm happy we have finally landed on it. Yes, indeed the new API > proposed here provides more flexibility, and it does so by embracing a > "data driven" API Vs the traditional procedural APIs we have seen for > *the firmware API*. This has been going on forever. Everybody hates your data-driven one. It's too indirect, it adds all those nasty "descriptors" of what to do, and it doesn't match what the current model does at all. Things like that may be ok as an internal implementation, but even there it's questionable if it then means a big disconnect between what people actually use (the normal functional model) and the implementation. The thing is, it's much better to just have functions that load the firmware data. Have them named that way ("load_firmware()"), and act that way ("just load the damn firmware file") instead of having odd descriptors that describe what is goign to be done and some state for it, and then get passed around. Don't add this kind of crazy abstraction complexity. If somebody wants to veryify a signature on a firmware file, they should *NOT* fill in a descriptor that says "check signature when loading". Thats' complete BS. They should just do "load_firmware()" and then "check_signature()" or whatever. Would such a "load firmware file, then check signature" take a few lines (with error handling)? Yes. But it is a *simple* interface. It doesn't have some stupid "struct driver_data_params" that needs to be filled in with random details (or has magic macros in a header file that fill in default values). It's _straightforward_. Linus
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 11:59:36PM +0800, Greg KH wrote: > On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > You may argue that *one* upstream users is not sufficient to introduce a new > > feature for, but I disagree given we have had new full *API* added for a new > > feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For > > instance request_firmware_into_buf() has no upstream users!!! > > That's not acceptable at all, I'll send a patch after this to remove > that. We don't keep apis around with no in-kernel users, you know this. I'm delighted to hear we can do away with the request_firmware_into_buf() crap. > > Now, you might say that even though this is true that there many users of > > out-of-tree drivers that need this. While true, if this is the bar we'd go > > with, we can't then ignore the iwlwifi userbase, and the possible gains of > > having a proper non-recursive use of the daisy chained requests. > > Nope, I don't care about out-of-tree drivers as we have no idea what is > going on there at all. I've always had this position. Beautiful. Music to my ears. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 11:59:36PM +0800, Greg KH wrote: > On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > You may argue that *one* upstream users is not sufficient to introduce a new > > feature for, but I disagree given we have had new full *API* added for a new > > feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For > > instance request_firmware_into_buf() has no upstream users!!! > > That's not acceptable at all, I'll send a patch after this to remove > that. We don't keep apis around with no in-kernel users, you know this. I'm delighted to hear we can do away with the request_firmware_into_buf() crap. > > Now, you might say that even though this is true that there many users of > > out-of-tree drivers that need this. While true, if this is the bar we'd go > > with, we can't then ignore the iwlwifi userbase, and the possible gains of > > having a proper non-recursive use of the daisy chained requests. > > Nope, I don't care about out-of-tree drivers as we have no idea what is > going on there at all. I've always had this position. Beautiful. Music to my ears. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 11:51:23PM +0800, Greg KH wrote: > On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > > I agree, that's what I'm saying here. I just do not see that happening > > > with your patch set at all. It's adding more code, a more complex way > > > to interact with the subsystem, and not making driver writer lives any > > > easier at all that I can see. > > > > There are two things to consider: > > > > a) The current design of the firmware API, and interfaces with exported > > symbols > > > > The case for the driver data API was that we were being super sloppy with > > extensions, > > to the point was making the internal code base very bug prone and full or > > redirect > > conditionals with #ifdefery nightmware stuff. > > > > b) Features of the firmware API > > > > These have to be evaluated on a case by case basis. > > Wait, no, you didn't address my main complaint at all here. You are > adding complexity for no perceived gain at all with this patch set. > > Now you might feel that this series gets you moving forward toward an > end goal of reduced complexity and wonderfulness, but you know how > kernel development works, you have to justify _all_ of your changes, not > just some future end result that is not even presented here. My point was that a) was already complex, so to say what I'm adding to address a) is complex would be unfair, so rather the question should be if its less complex, or if there are valid technical issues I'd like to hear them. > > > I, and others I know, have told you to work on simplifying your > responses, and descriptions, of patches. Take the extra time to make a > shorter answer. You will get better results, as I dread having to read > and respond to them currently. Sure, point taken! > I know you have spent a lot of time and effort on this work, but as it > stands, this crazy new interface (data-driven api vs. the traditional > procedural apis we know and love in Linux), is not acceptable at all. Ah, yes! Here is what I believe seems to be the *crux* issue of these patch series and I'm happy we have finally landed on it. Yes, indeed the new API proposed here provides more flexibility, and it does so by embracing a "data driven" API Vs the traditional procedural APIs we have seen for *the firmware API*. If by data-driven you mean using structs to drive the requirements, instead of adding more API per new feature. I would strongly disagree that we always prefer adding new functional APIs loosely. I think this should be reviewed on a case by case basis and it should be up to the maintainer who has better visibility into the history of the code, and what is coming. A quick git log grep found a recent commit example where we take on a flexible API on other *random* places in Linux. Refer to for example the change form bioset_create_nobvec() to bioset_create() and use flags in the patch titled "blk: replace bioset_create_nobvec() with a flags arg to bioset_create()", present on linux-next [0] followed by "blk: make the bioset rescue_workqueue optional." [1] which also extends the flags and uses them. To argue that we *still* need to keep doing a functional approach for the firmware API and keep adding new routines for new features, seems insane to me at this point -- if that is what you were suggesting... Also, the reason *why* I think this discussion is important is that it also implicates the amount of collateral evolutions needed per API. If you embrace data-driven API (or flags, or structs for APIs) you should see less patches due to collateral evolutions. In a way its my own resolution to mitigate *unnecessary* collateral evolutions by proper architecture. Where that line is drawn should be up to the designers of the API. For system calls, for instance, I think its well accepted we *never* want to make inflexible APIs. There is strong history for why that is the case. We cannot compare system calls to exported symbols, at all, but we can learn a bit from the flexibility efforts on them for exported symbols as well. Its *real news* to me that we *always* prefer a procedural preferences for APIs / exported symbols. [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=011067b05668b05aae88e5a24cff0ca0a67ca0b0 [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=47e0fb461fca1a68a566c82fcc006cc787312d8c > It's also blocking real bug fixes and features that people want > addressed, which isn't acceptable. What? What bugs ? I have addressed *every single* stable issue in queue and have sent patches for them and they do not depend in any way on any of this series! In fact I've taken liberty to go to great lengths to ensure *stable* proposals *get proper review* so we *do the right thing*. Case in point was the recent -ERESTART crap which in the end we ended up moving towards preferring adding new swait API for stable for it. I have ZERO stable fixes
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Fri, Jun 23, 2017 at 11:51:23PM +0800, Greg KH wrote: > On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > > I agree, that's what I'm saying here. I just do not see that happening > > > with your patch set at all. It's adding more code, a more complex way > > > to interact with the subsystem, and not making driver writer lives any > > > easier at all that I can see. > > > > There are two things to consider: > > > > a) The current design of the firmware API, and interfaces with exported > > symbols > > > > The case for the driver data API was that we were being super sloppy with > > extensions, > > to the point was making the internal code base very bug prone and full or > > redirect > > conditionals with #ifdefery nightmware stuff. > > > > b) Features of the firmware API > > > > These have to be evaluated on a case by case basis. > > Wait, no, you didn't address my main complaint at all here. You are > adding complexity for no perceived gain at all with this patch set. > > Now you might feel that this series gets you moving forward toward an > end goal of reduced complexity and wonderfulness, but you know how > kernel development works, you have to justify _all_ of your changes, not > just some future end result that is not even presented here. My point was that a) was already complex, so to say what I'm adding to address a) is complex would be unfair, so rather the question should be if its less complex, or if there are valid technical issues I'd like to hear them. > > > I, and others I know, have told you to work on simplifying your > responses, and descriptions, of patches. Take the extra time to make a > shorter answer. You will get better results, as I dread having to read > and respond to them currently. Sure, point taken! > I know you have spent a lot of time and effort on this work, but as it > stands, this crazy new interface (data-driven api vs. the traditional > procedural apis we know and love in Linux), is not acceptable at all. Ah, yes! Here is what I believe seems to be the *crux* issue of these patch series and I'm happy we have finally landed on it. Yes, indeed the new API proposed here provides more flexibility, and it does so by embracing a "data driven" API Vs the traditional procedural APIs we have seen for *the firmware API*. If by data-driven you mean using structs to drive the requirements, instead of adding more API per new feature. I would strongly disagree that we always prefer adding new functional APIs loosely. I think this should be reviewed on a case by case basis and it should be up to the maintainer who has better visibility into the history of the code, and what is coming. A quick git log grep found a recent commit example where we take on a flexible API on other *random* places in Linux. Refer to for example the change form bioset_create_nobvec() to bioset_create() and use flags in the patch titled "blk: replace bioset_create_nobvec() with a flags arg to bioset_create()", present on linux-next [0] followed by "blk: make the bioset rescue_workqueue optional." [1] which also extends the flags and uses them. To argue that we *still* need to keep doing a functional approach for the firmware API and keep adding new routines for new features, seems insane to me at this point -- if that is what you were suggesting... Also, the reason *why* I think this discussion is important is that it also implicates the amount of collateral evolutions needed per API. If you embrace data-driven API (or flags, or structs for APIs) you should see less patches due to collateral evolutions. In a way its my own resolution to mitigate *unnecessary* collateral evolutions by proper architecture. Where that line is drawn should be up to the designers of the API. For system calls, for instance, I think its well accepted we *never* want to make inflexible APIs. There is strong history for why that is the case. We cannot compare system calls to exported symbols, at all, but we can learn a bit from the flexibility efforts on them for exported symbols as well. Its *real news* to me that we *always* prefer a procedural preferences for APIs / exported symbols. [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=011067b05668b05aae88e5a24cff0ca0a67ca0b0 [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=47e0fb461fca1a68a566c82fcc006cc787312d8c > It's also blocking real bug fixes and features that people want > addressed, which isn't acceptable. What? What bugs ? I have addressed *every single* stable issue in queue and have sent patches for them and they do not depend in any way on any of this series! In fact I've taken liberty to go to great lengths to ensure *stable* proposals *get proper review* so we *do the right thing*. Case in point was the recent -ERESTART crap which in the end we ended up moving towards preferring adding new swait API for stable for it. I have ZERO stable fixes
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > I agree, that's what I'm saying here. I just do not see that happening > > with your patch set at all. It's adding more code, a more complex way > > to interact with the subsystem, and not making driver writer lives any > > easier at all that I can see. > > There are two things to consider: > > a) The current design of the firmware API, and interfaces with exported > symbols > > The case for the driver data API was that we were being super sloppy with > extensions, > to the point was making the internal code base very bug prone and full or > redirect > conditionals with #ifdefery nightmware stuff. > > b) Features of the firmware API > > These have to be evaluated on a case by case basis. Wait, no, you didn't address my main complaint at all here. You are adding complexity for no perceived gain at all with this patch set. Now you might feel that this series gets you moving forward toward an end goal of reduced complexity and wonderfulness, but you know how kernel development works, you have to justify _all_ of your changes, not just some future end result that is not even presented here. I, and others I know, have told you to work on simplifying your responses, and descriptions, of patches. Take the extra time to make a shorter answer. You will get better results, as I dread having to read and respond to them currently. I know you have spent a lot of time and effort on this work, but as it stands, this crazy new interface (data-driven api vs. the traditional procedural apis we know and love in Linux), is not acceptable at all. It's also blocking real bug fixes and features that people want addressed, which isn't acceptable. Please take the time to step back, and see if you really want to spend the effort into creating something that you can easily justify and break down into acceptable patches. If so, great, do it, but as it stands today, that is not what you have done here, at all. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > I agree, that's what I'm saying here. I just do not see that happening > > with your patch set at all. It's adding more code, a more complex way > > to interact with the subsystem, and not making driver writer lives any > > easier at all that I can see. > > There are two things to consider: > > a) The current design of the firmware API, and interfaces with exported > symbols > > The case for the driver data API was that we were being super sloppy with > extensions, > to the point was making the internal code base very bug prone and full or > redirect > conditionals with #ifdefery nightmware stuff. > > b) Features of the firmware API > > These have to be evaluated on a case by case basis. Wait, no, you didn't address my main complaint at all here. You are adding complexity for no perceived gain at all with this patch set. Now you might feel that this series gets you moving forward toward an end goal of reduced complexity and wonderfulness, but you know how kernel development works, you have to justify _all_ of your changes, not just some future end result that is not even presented here. I, and others I know, have told you to work on simplifying your responses, and descriptions, of patches. Take the extra time to make a shorter answer. You will get better results, as I dread having to read and respond to them currently. I know you have spent a lot of time and effort on this work, but as it stands, this crazy new interface (data-driven api vs. the traditional procedural apis we know and love in Linux), is not acceptable at all. It's also blocking real bug fixes and features that people want addressed, which isn't acceptable. Please take the time to step back, and see if you really want to spend the effort into creating something that you can easily justify and break down into acceptable patches. If so, great, do it, but as it stands today, that is not what you have done here, at all. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
To respond to one issue in your wall-of-text response: On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > You may argue that *one* upstream users is not sufficient to introduce a new > feature for, but I disagree given we have had new full *API* added for a new > feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For > instance request_firmware_into_buf() has no upstream users!!! That's not acceptable at all, I'll send a patch after this to remove that. We don't keep apis around with no in-kernel users, you know this. > Now, you might say that even though this is true that there many users of > out-of-tree drivers that need this. While true, if this is the bar we'd go > with, we can't then ignore the iwlwifi userbase, and the possible gains of > having a proper non-recursive use of the daisy chained requests. Nope, I don't care about out-of-tree drivers as we have no idea what is going on there at all. I've always had this position. Patch forthcoming. greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
To respond to one issue in your wall-of-text response: On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > You may argue that *one* upstream users is not sufficient to introduce a new > feature for, but I disagree given we have had new full *API* added for a new > feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For > instance request_firmware_into_buf() has no upstream users!!! That's not acceptable at all, I'll send a patch after this to remove that. We don't keep apis around with no in-kernel users, you know this. > Now, you might say that even though this is true that there many users of > out-of-tree drivers that need this. While true, if this is the bar we'd go > with, we can't then ignore the iwlwifi userbase, and the possible gains of > having a proper non-recursive use of the daisy chained requests. Nope, I don't care about out-of-tree drivers as we have no idea what is going on there at all. I've always had this position. Patch forthcoming. greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Wed, Jun 21, 2017 at 09:49:35AM +0900, AKASHI Takahiro wrote: > On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > > > > > perhaps a light > > > internal rework inside firmware_class might be more suitable towards the > > > extensibility goal while keeping driver API usage as simple as it is today > > > (even if you hate my patch for various reasons)? > > > > > > [1] - > > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > > > > What you did is but one step I took, your changes make it easier to shuffle > > data around internally. Its not sufficient to clean things up well enough, > > for > > instance look at the "firmware behavior options" which are cleaned up in > > this > > patch 1/5. That's been one pain on our side for a while, people > > understanding > > when a flag applies or a feature, and making sure we document it all. > > > > Then, one of the end goals is to provide extensibility, this is to allow > > users > > to *pass* similar type of struct for either a sync or async call. Without > > this > > the API remains unflexible and I predict we'll end up with the same > > situation > > later anyway. > > > > The approach I took uses an internal struct for passing required data for > > the > > private internal API use. Then it also provides a public struct which can be > > used to grow requirements to make only *new* API easily extensible. > > > > So we need all three things to move forward. > > Given the discussions here, it would be better to split your [1/5] and > [2/5] into more smaller pieces, say, > * re-factor the old APIs with introducing private fw_desc So patch 1/5 introduces 3 structs: o struct driver_data_req_params - used for user specified parameters o struct driver_data_priv_params - used for internal use only o struct driver_data_params - stiches both of the above together, only for internal use I could certainly split the patch to introduce each separately. > * add new APIs with extra public arguments This was split out already, patch 2 is the first patch introducing new API. > * extend new APIs per-feature > - sync/async callbacks I suppose the base would be what we have today, only in new form. And sure, I can add the features one by one... > - API version, and so on Right. > This way, you can illustrate how your approach evolves and it may > mitigate people's concern about how complicated it is on the surface, > allowing for discussing each of aspects of new APIs separately. This makes sense. Greg, does this make sense to you? Or are you stone cold against all this? Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Wed, Jun 21, 2017 at 09:49:35AM +0900, AKASHI Takahiro wrote: > On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > > > > > perhaps a light > > > internal rework inside firmware_class might be more suitable towards the > > > extensibility goal while keeping driver API usage as simple as it is today > > > (even if you hate my patch for various reasons)? > > > > > > [1] - > > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > > > > What you did is but one step I took, your changes make it easier to shuffle > > data around internally. Its not sufficient to clean things up well enough, > > for > > instance look at the "firmware behavior options" which are cleaned up in > > this > > patch 1/5. That's been one pain on our side for a while, people > > understanding > > when a flag applies or a feature, and making sure we document it all. > > > > Then, one of the end goals is to provide extensibility, this is to allow > > users > > to *pass* similar type of struct for either a sync or async call. Without > > this > > the API remains unflexible and I predict we'll end up with the same > > situation > > later anyway. > > > > The approach I took uses an internal struct for passing required data for > > the > > private internal API use. Then it also provides a public struct which can be > > used to grow requirements to make only *new* API easily extensible. > > > > So we need all three things to move forward. > > Given the discussions here, it would be better to split your [1/5] and > [2/5] into more smaller pieces, say, > * re-factor the old APIs with introducing private fw_desc So patch 1/5 introduces 3 structs: o struct driver_data_req_params - used for user specified parameters o struct driver_data_priv_params - used for internal use only o struct driver_data_params - stiches both of the above together, only for internal use I could certainly split the patch to introduce each separately. > * add new APIs with extra public arguments This was split out already, patch 2 is the first patch introducing new API. > * extend new APIs per-feature > - sync/async callbacks I suppose the base would be what we have today, only in new form. And sure, I can add the features one by one... > - API version, and so on Right. > This way, you can illustrate how your approach evolves and it may > mitigate people's concern about how complicated it is on the surface, > allowing for discussing each of aspects of new APIs separately. This makes sense. Greg, does this make sense to you? Or are you stone cold against all this? Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > > > perhaps a light > > internal rework inside firmware_class might be more suitable towards the > > extensibility goal while keeping driver API usage as simple as it is today > > (even if you hate my patch for various reasons)? > > > > [1] - > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > > What you did is but one step I took, your changes make it easier to shuffle > data around internally. Its not sufficient to clean things up well enough, for > instance look at the "firmware behavior options" which are cleaned up in this > patch 1/5. That's been one pain on our side for a while, people understanding > when a flag applies or a feature, and making sure we document it all. > > Then, one of the end goals is to provide extensibility, this is to allow users > to *pass* similar type of struct for either a sync or async call. Without this > the API remains unflexible and I predict we'll end up with the same situation > later anyway. > > The approach I took uses an internal struct for passing required data for the > private internal API use. Then it also provides a public struct which can be > used to grow requirements to make only *new* API easily extensible. > > So we need all three things to move forward. Given the discussions here, it would be better to split your [1/5] and [2/5] into more smaller pieces, say, * re-factor the old APIs with introducing private fw_desc * add new APIs with extra public arguments * extend new APIs per-feature - sync/async callbacks - API version, and so on This way, you can illustrate how your approach evolves and it may mitigate people's concern about how complicated it is on the surface, allowing for discussing each of aspects of new APIs separately. Thanks, -Takahiro AKASHI > Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > > > perhaps a light > > internal rework inside firmware_class might be more suitable towards the > > extensibility goal while keeping driver API usage as simple as it is today > > (even if you hate my patch for various reasons)? > > > > [1] - > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 > > What you did is but one step I took, your changes make it easier to shuffle > data around internally. Its not sufficient to clean things up well enough, for > instance look at the "firmware behavior options" which are cleaned up in this > patch 1/5. That's been one pain on our side for a while, people understanding > when a flag applies or a feature, and making sure we document it all. > > Then, one of the end goals is to provide extensibility, this is to allow users > to *pass* similar type of struct for either a sync or async call. Without this > the API remains unflexible and I predict we'll end up with the same situation > later anyway. > > The approach I took uses an internal struct for passing required data for the > private internal API use. Then it also provides a public struct which can be > used to grow requirements to make only *new* API easily extensible. > > So we need all three things to move forward. Given the discussions here, it would be better to split your [1/5] and [2/5] into more smaller pieces, say, * re-factor the old APIs with introducing private fw_desc * add new APIs with extra public arguments * extend new APIs per-feature - sync/async callbacks - API version, and so on This way, you can illustrate how your approach evolves and it may mitigate people's concern about how complicated it is on the surface, allowing for discussing each of aspects of new APIs separately. Thanks, -Takahiro AKASHI > Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > Hi Greg, Luis, > > On 2017-06-17 12:38, Greg KH wrote: > > On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > > > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > > As the firmware API evolves we keep extending functions with more > > > > > arguments. > > > > > Stop this nonsense by proving an extensible data structure which can > > > > > be used > > > > > to represent both user parameters and private internal parameters. > > > > > > > > Let's take a simple C function interface and make it a more complex > > > > data-driven interface that is impossible to understand and obviously > > > > understand how it is to be used and works! > > > > > > The firmware codebase was already complex! > > > > Heh, I'm not arguing with you there :) > > > > > What you have to ask yourself really is if this makes it *less > > > complex* and > > > helps *clean things up* in a much better way than it was before. > > > Also does it > > > allow us to *pave the way for new functionality easily*, without > > > creating > > > further mess? > > > > I agree, that's what I'm saying here. I just do not see that happening > > with your patch set at all. It's adding more code, a more complex way > > to interact with the subsystem, and not making driver writer lives any > > easier at all that I can see. > > > > Again, the code is now bigger, does more, with not even any real benefit > > for existing users. > > > > > If not, what concrete alternatives do you suggest? > > > > It's working, so leave it alone? :) > > > > So I wanted to note here that I had a similar internal discussion with > Stephen Boyd when he first upstreamed the request_firmware_into_buf API. Thanks for sharing! > I actually did change things a bit to pass around a 'desc' structure with all > the flags and parameters in our internal vendor tree [1]. I'll note the "desc" approach and name was what I originally used also on my earlier incarnations of the driver data API (back then the "sysdata API"). > This is a sort of > an ultra-lite version of what Luis is trying to do - extensibility - but does > not change the API for driver owners. What I propose does not change the API for existing drivers either. The changes I propose allows the flexibility both internally and externally but only externally for newer APIs and features. > Existing APIs become wrappers around > the new API. My primary reason then was the number of things being passed > around between layers of functions inside firmware_class seemed a bit > untenable. Yup!! Same applies to the public API! > Just like Greg, Stephen also brought up the argument about why the > unnecessary complication to the API without any measurable benefit - this > seemed a fair argument to me at that time. But here's my point - if Luis > agrees that _this_ patchset is going slightly Mjolnir, One of the issues you'd face with your changes is you are essentially diverging now with *similar* cleanup but its *not upstream*, so fixes may require manual manipulation! The place to address what you need is not a private vendor tree but *upstream* otherwise your tree will diverge and can make it hard for you to merge stable fixes or cherry pick enhancements. The fact that folks are willing to go the such lengths as to *merge* side internal cleanups to internal APIs should say a lot of confirming the need for my own work. I stand by this work as needed, I have been the one doing the heavy lifting of addressing fixes, documenting it and looking at a real sensible way to extend it, as such I really do believe its needed. I'm tired of seeing side hacks and people throwing gum at things. > perhaps a light > internal rework inside firmware_class might be more suitable towards the > extensibility goal while keeping driver API usage as simple as it is today > (even if you hate my patch for various reasons)? > > [1] - > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 What you did is but one step I took, your changes make it easier to shuffle data around internally. Its not sufficient to clean things up well enough, for instance look at the "firmware behavior options" which are cleaned up in this patch 1/5. That's been one pain on our side for a while, people understanding when a flag applies or a feature, and making sure we document it all. Then, one of the end goals is to provide extensibility, this is to allow users to *pass* similar type of struct for either a sync or async call. Without this the API remains unflexible and I predict we'll end up with the same situation later anyway. The approach I took uses an internal struct for passing required data for the private internal API use. Then it also provides a public struct which can be used to
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > Hi Greg, Luis, > > On 2017-06-17 12:38, Greg KH wrote: > > On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > > > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > > As the firmware API evolves we keep extending functions with more > > > > > arguments. > > > > > Stop this nonsense by proving an extensible data structure which can > > > > > be used > > > > > to represent both user parameters and private internal parameters. > > > > > > > > Let's take a simple C function interface and make it a more complex > > > > data-driven interface that is impossible to understand and obviously > > > > understand how it is to be used and works! > > > > > > The firmware codebase was already complex! > > > > Heh, I'm not arguing with you there :) > > > > > What you have to ask yourself really is if this makes it *less > > > complex* and > > > helps *clean things up* in a much better way than it was before. > > > Also does it > > > allow us to *pave the way for new functionality easily*, without > > > creating > > > further mess? > > > > I agree, that's what I'm saying here. I just do not see that happening > > with your patch set at all. It's adding more code, a more complex way > > to interact with the subsystem, and not making driver writer lives any > > easier at all that I can see. > > > > Again, the code is now bigger, does more, with not even any real benefit > > for existing users. > > > > > If not, what concrete alternatives do you suggest? > > > > It's working, so leave it alone? :) > > > > So I wanted to note here that I had a similar internal discussion with > Stephen Boyd when he first upstreamed the request_firmware_into_buf API. Thanks for sharing! > I actually did change things a bit to pass around a 'desc' structure with all > the flags and parameters in our internal vendor tree [1]. I'll note the "desc" approach and name was what I originally used also on my earlier incarnations of the driver data API (back then the "sysdata API"). > This is a sort of > an ultra-lite version of what Luis is trying to do - extensibility - but does > not change the API for driver owners. What I propose does not change the API for existing drivers either. The changes I propose allows the flexibility both internally and externally but only externally for newer APIs and features. > Existing APIs become wrappers around > the new API. My primary reason then was the number of things being passed > around between layers of functions inside firmware_class seemed a bit > untenable. Yup!! Same applies to the public API! > Just like Greg, Stephen also brought up the argument about why the > unnecessary complication to the API without any measurable benefit - this > seemed a fair argument to me at that time. But here's my point - if Luis > agrees that _this_ patchset is going slightly Mjolnir, One of the issues you'd face with your changes is you are essentially diverging now with *similar* cleanup but its *not upstream*, so fixes may require manual manipulation! The place to address what you need is not a private vendor tree but *upstream* otherwise your tree will diverge and can make it hard for you to merge stable fixes or cherry pick enhancements. The fact that folks are willing to go the such lengths as to *merge* side internal cleanups to internal APIs should say a lot of confirming the need for my own work. I stand by this work as needed, I have been the one doing the heavy lifting of addressing fixes, documenting it and looking at a real sensible way to extend it, as such I really do believe its needed. I'm tired of seeing side hacks and people throwing gum at things. > perhaps a light > internal rework inside firmware_class might be more suitable towards the > extensibility goal while keeping driver API usage as simple as it is today > (even if you hate my patch for various reasons)? > > [1] - > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319 What you did is but one step I took, your changes make it easier to shuffle data around internally. Its not sufficient to clean things up well enough, for instance look at the "firmware behavior options" which are cleaned up in this patch 1/5. That's been one pain on our side for a while, people understanding when a flag applies or a feature, and making sure we document it all. Then, one of the end goals is to provide extensibility, this is to allow users to *pass* similar type of struct for either a sync or async call. Without this the API remains unflexible and I predict we'll end up with the same situation later anyway. The approach I took uses an internal struct for passing required data for the private internal API use. Then it also provides a public struct which can be used to
Re: [PATCH v9 1/5] firmware: add extensible driver data params
Hi Greg, Luis, On 2017-06-17 12:38, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > As the firmware API evolves we keep extending functions with more arguments. > > Stop this nonsense by proving an extensible data structure which can be used > > to represent both user parameters and private internal parameters. > > Let's take a simple C function interface and make it a more complex > data-driven interface that is impossible to understand and obviously > understand how it is to be used and works! The firmware codebase was already complex! Heh, I'm not arguing with you there :) What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) So I wanted to note here that I had a similar internal discussion with Stephen Boyd when he first upstreamed the request_firmware_into_buf API. I actually did change things a bit to pass around a 'desc' structure with all the flags and parameters in our internal vendor tree [1]. This is a sort of an ultra-lite version of what Luis is trying to do - extensibility - but does not change the API for driver owners. Existing APIs become wrappers around the new API. My primary reason then was the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable. Just like Greg, Stephen also brought up the argument about why the unnecessary complication to the API without any measurable benefit - this seemed a fair argument to me at that time. But here's my point - if Luis agrees that _this_ patchset is going slightly Mjolnir, perhaps a light internal rework inside firmware_class might be more suitable towards the extensibility goal while keeping driver API usage as simple as it is today (even if you hate my patch for various reasons)? Thanks, Vikram [1] - https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319
Re: [PATCH v9 1/5] firmware: add extensible driver data params
Hi Greg, Luis, On 2017-06-17 12:38, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > As the firmware API evolves we keep extending functions with more arguments. > > Stop this nonsense by proving an extensible data structure which can be used > > to represent both user parameters and private internal parameters. > > Let's take a simple C function interface and make it a more complex > data-driven interface that is impossible to understand and obviously > understand how it is to be used and works! The firmware codebase was already complex! Heh, I'm not arguing with you there :) What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) So I wanted to note here that I had a similar internal discussion with Stephen Boyd when he first upstreamed the request_firmware_into_buf API. I actually did change things a bit to pass around a 'desc' structure with all the flags and parameters in our internal vendor tree [1]. This is a sort of an ultra-lite version of what Luis is trying to do - extensibility - but does not change the API for driver owners. Existing APIs become wrappers around the new API. My primary reason then was the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable. Just like Greg, Stephen also brought up the argument about why the unnecessary complication to the API without any measurable benefit - this seemed a fair argument to me at that time. But here's my point - if Luis agrees that _this_ patchset is going slightly Mjolnir, perhaps a light internal rework inside firmware_class might be more suitable towards the extensibility goal while keeping driver API usage as simple as it is today (even if you hate my patch for various reasons)? Thanks, Vikram [1] - https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18=7aa7efd3c150840369739893a84bd1d9f9774319
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/19/2017 8:48 PM, AKASHI Takahiro wrote: On Mon, Jun 19, 2017 at 05:51:08PM -0500, Li, Yi wrote: Hi Greg, On 6/17/2017 2:38 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. I am still new to the upstreaming world, pardon me if my understanding is naive. :) My take with Luis's driver data API is that it adds a wrapper on top of the old request_firmware APIs, so the new features can be added/disabled by the parameters structures instead of adding/changing API functions. Agree that there is not much new for existing users. It adds more codes (not necessary more complex) but create a consistent way for extension IMO. Most of code of my feature, firmware signing, is implemented in common place between old and new APIs, while only new API has a parameter, DRIVER_DATA_REQ_NO_SIG_CHECK, which allow users to enable/disable this feature per-driver-datum. Simple enough. I meant to say more code does NOT necessary equal to more complex, sorry for the confusion. So what matters is adding yet another variant of request_firmware_xx() vs. adding a mere parameter? Agree, I also prefer the parameter approach. Thanks, -Takahiro AKASHI Below are 3 examples I tried to add streaming support to load large firmware files. Adding streaming with driver data API: https://patchwork.kernel.org/patch/9738503 . This patch series depends on non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is bigger than it should be since it added some codes to test firmware caching. and pre-allocate buffer patch series https://patchwork.kernel.org/patch/9738487/ By comparison, here is my old streaming RFC with original firmware class: https://lkml.org/lkml/2017/3/9/872 Do you think this is the better way? Thanks, Yi
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/19/2017 8:48 PM, AKASHI Takahiro wrote: On Mon, Jun 19, 2017 at 05:51:08PM -0500, Li, Yi wrote: Hi Greg, On 6/17/2017 2:38 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. I am still new to the upstreaming world, pardon me if my understanding is naive. :) My take with Luis's driver data API is that it adds a wrapper on top of the old request_firmware APIs, so the new features can be added/disabled by the parameters structures instead of adding/changing API functions. Agree that there is not much new for existing users. It adds more codes (not necessary more complex) but create a consistent way for extension IMO. Most of code of my feature, firmware signing, is implemented in common place between old and new APIs, while only new API has a parameter, DRIVER_DATA_REQ_NO_SIG_CHECK, which allow users to enable/disable this feature per-driver-datum. Simple enough. I meant to say more code does NOT necessary equal to more complex, sorry for the confusion. So what matters is adding yet another variant of request_firmware_xx() vs. adding a mere parameter? Agree, I also prefer the parameter approach. Thanks, -Takahiro AKASHI Below are 3 examples I tried to add streaming support to load large firmware files. Adding streaming with driver data API: https://patchwork.kernel.org/patch/9738503 . This patch series depends on non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is bigger than it should be since it added some codes to test firmware caching. and pre-allocate buffer patch series https://patchwork.kernel.org/patch/9738487/ By comparison, here is my old streaming RFC with original firmware class: https://lkml.org/lkml/2017/3/9/872 Do you think this is the better way? Thanks, Yi
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 05:51:08PM -0500, Li, Yi wrote: > Hi Greg, > > On 6/17/2017 2:38 PM, Greg KH wrote: > >On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > >>On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > >>>On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > >>What you have to ask yourself really is if this makes it *less complex* and > >>helps *clean things up* in a much better way than it was before. Also does > >>it > >>allow us to *pave the way for new functionality easily*, without creating > >>further mess? > > > >I agree, that's what I'm saying here. I just do not see that happening > >with your patch set at all. It's adding more code, a more complex way > >to interact with the subsystem, and not making driver writer lives any > >easier at all that I can see. > > > >Again, the code is now bigger, does more, with not even any real benefit > >for existing users. > > I am still new to the upstreaming world, pardon me if my understanding is > naive. :) My take with Luis's driver data API is that it adds a wrapper on > top of the old request_firmware APIs, so the new features can be > added/disabled by the parameters structures instead of adding/changing API > functions. Agree that there is not much new for existing users. It adds more > codes (not necessary more complex) but create a consistent way for extension > IMO. Most of code of my feature, firmware signing, is implemented in common place between old and new APIs, while only new API has a parameter, DRIVER_DATA_REQ_NO_SIG_CHECK, which allow users to enable/disable this feature per-driver-datum. Simple enough. So what matters is adding yet another variant of request_firmware_xx() vs. adding a mere parameter? Thanks, -Takahiro AKASHI > Below are 3 examples I tried to add streaming support to load large firmware > files. > Adding streaming with driver data API: > https://patchwork.kernel.org/patch/9738503 . This patch series depends on > non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is > bigger than it should be since it added some codes to test firmware caching. > and pre-allocate buffer patch series > https://patchwork.kernel.org/patch/9738487/ > > By comparison, here is my old streaming RFC with original firmware class: > https://lkml.org/lkml/2017/3/9/872 > Do you think this is the better way? > > Thanks, > Yi
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 05:51:08PM -0500, Li, Yi wrote: > Hi Greg, > > On 6/17/2017 2:38 PM, Greg KH wrote: > >On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > >>On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > >>>On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > >>What you have to ask yourself really is if this makes it *less complex* and > >>helps *clean things up* in a much better way than it was before. Also does > >>it > >>allow us to *pave the way for new functionality easily*, without creating > >>further mess? > > > >I agree, that's what I'm saying here. I just do not see that happening > >with your patch set at all. It's adding more code, a more complex way > >to interact with the subsystem, and not making driver writer lives any > >easier at all that I can see. > > > >Again, the code is now bigger, does more, with not even any real benefit > >for existing users. > > I am still new to the upstreaming world, pardon me if my understanding is > naive. :) My take with Luis's driver data API is that it adds a wrapper on > top of the old request_firmware APIs, so the new features can be > added/disabled by the parameters structures instead of adding/changing API > functions. Agree that there is not much new for existing users. It adds more > codes (not necessary more complex) but create a consistent way for extension > IMO. Most of code of my feature, firmware signing, is implemented in common place between old and new APIs, while only new API has a parameter, DRIVER_DATA_REQ_NO_SIG_CHECK, which allow users to enable/disable this feature per-driver-datum. Simple enough. So what matters is adding yet another variant of request_firmware_xx() vs. adding a mere parameter? Thanks, -Takahiro AKASHI > Below are 3 examples I tried to add streaming support to load large firmware > files. > Adding streaming with driver data API: > https://patchwork.kernel.org/patch/9738503 . This patch series depends on > non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is > bigger than it should be since it added some codes to test firmware caching. > and pre-allocate buffer patch series > https://patchwork.kernel.org/patch/9738487/ > > By comparison, here is my old streaming RFC with original firmware class: > https://lkml.org/lkml/2017/3/9/872 > Do you think this is the better way? > > Thanks, > Yi
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:41:07PM +0200, Luis R. Rodriguez wrote: > On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote: > > On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > > > > > But we don't accept kernel patches for some mythical future option > > > that might be happening some time in the future. Heck, I'm still not > > > convinced that firmware signing isn't anything more than just some > > > snakeoil in the first place! > > > > I for one really want the "firmware" signing, because I want to load > > the regulatory database through this API, and > > This was my original goal as well... and it was also one of the reasons why > the API name change would be much better reflective of future possible uses. > > > But honestly, I've been waiting for years for that now and started > > looking at what it would take to hand-implement that on top of the > > existing firmware API. Probably not all that much. > > I had proposed changes to do just this long ago, without any new *API*, so > we'd > support firmware signing just as we do with module signing. Simple! > > It was during these discussions that we realized we actually *wanted* to have > the option to always specify requests with specific signing requirements from > the start, as such a flexible API became a prerequisite and so I prioritized > that work first. > > Lets not ignore previous work and prior discussions then, the last effort on > this > front was by AKASHI, and it'd be greatly appreciated if the topic of firmware > signing was specifically addressed on that thread there [0]. +1 I always appreciate any comments from those who are for and against my patch (or firmware signing in general) as well. -Takahiro AKASHI > [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.aka...@linaro.org > > Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:41:07PM +0200, Luis R. Rodriguez wrote: > On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote: > > On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > > > > > But we don't accept kernel patches for some mythical future option > > > that might be happening some time in the future. Heck, I'm still not > > > convinced that firmware signing isn't anything more than just some > > > snakeoil in the first place! > > > > I for one really want the "firmware" signing, because I want to load > > the regulatory database through this API, and > > This was my original goal as well... and it was also one of the reasons why > the API name change would be much better reflective of future possible uses. > > > But honestly, I've been waiting for years for that now and started > > looking at what it would take to hand-implement that on top of the > > existing firmware API. Probably not all that much. > > I had proposed changes to do just this long ago, without any new *API*, so > we'd > support firmware signing just as we do with module signing. Simple! > > It was during these discussions that we realized we actually *wanted* to have > the option to always specify requests with specific signing requirements from > the start, as such a flexible API became a prerequisite and so I prioritized > that work first. > > Lets not ignore previous work and prior discussions then, the last effort on > this > front was by AKASHI, and it'd be greatly appreciated if the topic of firmware > signing was specifically addressed on that thread there [0]. +1 I always appreciate any comments from those who are for and against my patch (or firmware signing in general) as well. -Takahiro AKASHI > [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.aka...@linaro.org > > Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
Hi Greg, On 6/17/2017 2:38 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! The firmware codebase was already complex! Heh, I'm not arguing with you there :) What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. I am still new to the upstreaming world, pardon me if my understanding is naive. :) My take with Luis's driver data API is that it adds a wrapper on top of the old request_firmware APIs, so the new features can be added/disabled by the parameters structures instead of adding/changing API functions. Agree that there is not much new for existing users. It adds more codes (not necessary more complex) but create a consistent way for extension IMO. Below are 3 examples I tried to add streaming support to load large firmware files. Adding streaming with driver data API: https://patchwork.kernel.org/patch/9738503 . This patch series depends on non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is bigger than it should be since it added some codes to test firmware caching. and pre-allocate buffer patch series https://patchwork.kernel.org/patch/9738487/ By comparison, here is my old streaming RFC with original firmware class: https://lkml.org/lkml/2017/3/9/872 Do you think this is the better way? Thanks, Yi If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) :( Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! But we don't accept kernel patches for some mythical future option that might be happening some time in the future. Heck, I'm still not convinced that firmware signing isn't anything more than just some snakeoil in the first place! So while you mention lots of times that all sorts of wonderful things can now possibly be built on top of the new code, I have yet to see it (meaning you didn't include it in the patch series.) To get me to take these changes, you have to show a real need and user of the code. Without that it just strongly looks like you are having fun making a more complex api for no reason that we are then going to be stuck with maintaining. So clean away, and fix up, but remember, you have to be able to justify each change as being needed. And so far, I'm not sold on this at all, sorry. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
Hi Greg, On 6/17/2017 2:38 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! The firmware codebase was already complex! Heh, I'm not arguing with you there :) What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. I am still new to the upstreaming world, pardon me if my understanding is naive. :) My take with Luis's driver data API is that it adds a wrapper on top of the old request_firmware APIs, so the new features can be added/disabled by the parameters structures instead of adding/changing API functions. Agree that there is not much new for existing users. It adds more codes (not necessary more complex) but create a consistent way for extension IMO. Below are 3 examples I tried to add streaming support to load large firmware files. Adding streaming with driver data API: https://patchwork.kernel.org/patch/9738503 . This patch series depends on non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is bigger than it should be since it added some codes to test firmware caching. and pre-allocate buffer patch series https://patchwork.kernel.org/patch/9738487/ By comparison, here is my old streaming RFC with original firmware class: https://lkml.org/lkml/2017/3/9/872 Do you think this is the better way? Thanks, Yi If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) :( Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! But we don't accept kernel patches for some mythical future option that might be happening some time in the future. Heck, I'm still not convinced that firmware signing isn't anything more than just some snakeoil in the first place! So while you mention lots of times that all sorts of wonderful things can now possibly be built on top of the new code, I have yet to see it (meaning you didn't include it in the patch series.) To get me to take these changes, you have to show a real need and user of the code. Without that it just strongly looks like you are having fun making a more complex api for no reason that we are then going to be stuck with maintaining. So clean away, and fix up, but remember, you have to be able to justify each change as being needed. And so far, I'm not sold on this at all, sorry. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote: > On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > > > But we don't accept kernel patches for some mythical future option > > that might be happening some time in the future. Heck, I'm still not > > convinced that firmware signing isn't anything more than just some > > snakeoil in the first place! > > I for one really want the "firmware" signing, because I want to load > the regulatory database through this API, and This was my original goal as well... and it was also one of the reasons why the API name change would be much better reflective of future possible uses. > But honestly, I've been waiting for years for that now and started > looking at what it would take to hand-implement that on top of the > existing firmware API. Probably not all that much. I had proposed changes to do just this long ago, without any new *API*, so we'd support firmware signing just as we do with module signing. Simple! It was during these discussions that we realized we actually *wanted* to have the option to always specify requests with specific signing requirements from the start, as such a flexible API became a prerequisite and so I prioritized that work first. Lets not ignore previous work and prior discussions then, the last effort on this front was by AKASHI, and it'd be greatly appreciated if the topic of firmware signing was specifically addressed on that thread there [0]. [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.aka...@linaro.org Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote: > On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > > > But we don't accept kernel patches for some mythical future option > > that might be happening some time in the future. Heck, I'm still not > > convinced that firmware signing isn't anything more than just some > > snakeoil in the first place! > > I for one really want the "firmware" signing, because I want to load > the regulatory database through this API, and This was my original goal as well... and it was also one of the reasons why the API name change would be much better reflective of future possible uses. > But honestly, I've been waiting for years for that now and started > looking at what it would take to hand-implement that on top of the > existing firmware API. Probably not all that much. I had proposed changes to do just this long ago, without any new *API*, so we'd support firmware signing just as we do with module signing. Simple! It was during these discussions that we realized we actually *wanted* to have the option to always specify requests with specific signing requirements from the start, as such a flexible API became a prerequisite and so I prioritized that work first. Lets not ignore previous work and prior discussions then, the last effort on this front was by AKASHI, and it'd be greatly appreciated if the topic of firmware signing was specifically addressed on that thread there [0]. [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.aka...@linaro.org Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 17, 2017 at 09:38:15PM +0200, Greg KH wrote: > On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > As the firmware API evolves we keep extending functions with more > > > > arguments. > > > > Stop this nonsense by proving an extensible data structure which can be > > > > used > > > > to represent both user parameters and private internal parameters. > > > > > > Let's take a simple C function interface and make it a more complex > > > data-driven interface that is impossible to understand and obviously > > > understand how it is to be used and works! > > > > The firmware codebase was already complex! > > Heh, I'm not arguing with you there :) Great! > > What you have to ask yourself really is if this makes it *less complex* and > > helps *clean things up* in a much better way than it was before. Also does > > it > > allow us to *pave the way for new functionality easily*, without creating > > further mess? > > I agree, that's what I'm saying here. I just do not see that happening > with your patch set at all. It's adding more code, a more complex way > to interact with the subsystem, and not making driver writer lives any > easier at all that I can see. There are two things to consider: a) The current design of the firmware API, and interfaces with exported symbols The case for the driver data API was that we were being super sloppy with extensions, to the point was making the internal code base very bug prone and full or redirect conditionals with #ifdefery nightmware stuff. b) Features of the firmware API These have to be evaluated on a case by case basis. > Again, the code is now bigger, does more, with not even any real benefit > for existing users. Obviously I disagree strongly, in light of the history of the code. Not only should the existing code be compared but also how it has evolved and we should evaluate whether or not its evolution can be dealt with more appropriately. > > If not, what concrete alternatives do you suggest? > > It's working, so leave it alone? :) As the maintainer of the firmware API I **totally** disagree!! First the firmware API has been evolving as pieces of gum thrown at it. A proper design of the API in consideration of extensions is in order. Likewise goes for testing and documentation. Second, it cannot be said with any serious tone that the current design has worked well. It is simply not an accurate reflection of the codebase. The design of the fallback mechanism and the amount of issues that have gone through it is a great example. The issues I have fixed along the way, and the chaotic way in which the API has evolved have put me to reflect well on a proper design of the firmware API. > > > :( > > > > > > Seriously, why? Why are we extending any of this at all? > > > > Addressing easy extensibility was a prerequisite of considering firmware > > signing > > support. Its really the *only* reason I started reviewing the firmware API, > > to the > > point I started helping fix quite a bit of bugs and now just became the > > maintainer. > > > > So my original firmware signing effort, now driven by AKASHI Takahiro, was > > one > > of the original motivations here! > > But we don't accept kernel patches for some mythical future option that > might be happening some time in the future. Greg, these are non mythical "future options" -- they are real feature requests and they have all valid reasons and discussions ongoing on the mailing list. The streaming FPGA support is a concrete valid use case we need to extend *now* and your reply brilliantly seems to have completely ignored it. I do agree that firmware signing itself was a largely debatable topic, there were *really-really* long threads on the topic. However we had a hallway track at Santa Fe Plumbers where it seemed we reached consensus on a path forward! The firmware signing topic should be addressed on TAKASHI's patch set [0]. Please address your NACK and reasons there. Even though it is based on the driver data API, the main topic points of *logic* for it can *also* be discussed there. BUT NOTE: Before we could move forward with firmware signing we need also a *sane* way to extend the API to address the needs of firmware signing. So a proper sane API for future extensions is in order as a prerequisite. This goes along with testing and documentation. This goes for *any* new firmware API feature!!! Firmware signing was just one example feature!!! Also -- even if they were "mythical" the *point* of this series is to address extensibility of the API, and so one of the ways to measure the gains of the design of a new API is to look at what the code would look like when new features are added. This is why I pointed out to review these proposed changes. Even more so given I am noting that these are non-mythical
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, Jun 17, 2017 at 09:38:15PM +0200, Greg KH wrote: > On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > As the firmware API evolves we keep extending functions with more > > > > arguments. > > > > Stop this nonsense by proving an extensible data structure which can be > > > > used > > > > to represent both user parameters and private internal parameters. > > > > > > Let's take a simple C function interface and make it a more complex > > > data-driven interface that is impossible to understand and obviously > > > understand how it is to be used and works! > > > > The firmware codebase was already complex! > > Heh, I'm not arguing with you there :) Great! > > What you have to ask yourself really is if this makes it *less complex* and > > helps *clean things up* in a much better way than it was before. Also does > > it > > allow us to *pave the way for new functionality easily*, without creating > > further mess? > > I agree, that's what I'm saying here. I just do not see that happening > with your patch set at all. It's adding more code, a more complex way > to interact with the subsystem, and not making driver writer lives any > easier at all that I can see. There are two things to consider: a) The current design of the firmware API, and interfaces with exported symbols The case for the driver data API was that we were being super sloppy with extensions, to the point was making the internal code base very bug prone and full or redirect conditionals with #ifdefery nightmware stuff. b) Features of the firmware API These have to be evaluated on a case by case basis. > Again, the code is now bigger, does more, with not even any real benefit > for existing users. Obviously I disagree strongly, in light of the history of the code. Not only should the existing code be compared but also how it has evolved and we should evaluate whether or not its evolution can be dealt with more appropriately. > > If not, what concrete alternatives do you suggest? > > It's working, so leave it alone? :) As the maintainer of the firmware API I **totally** disagree!! First the firmware API has been evolving as pieces of gum thrown at it. A proper design of the API in consideration of extensions is in order. Likewise goes for testing and documentation. Second, it cannot be said with any serious tone that the current design has worked well. It is simply not an accurate reflection of the codebase. The design of the fallback mechanism and the amount of issues that have gone through it is a great example. The issues I have fixed along the way, and the chaotic way in which the API has evolved have put me to reflect well on a proper design of the firmware API. > > > :( > > > > > > Seriously, why? Why are we extending any of this at all? > > > > Addressing easy extensibility was a prerequisite of considering firmware > > signing > > support. Its really the *only* reason I started reviewing the firmware API, > > to the > > point I started helping fix quite a bit of bugs and now just became the > > maintainer. > > > > So my original firmware signing effort, now driven by AKASHI Takahiro, was > > one > > of the original motivations here! > > But we don't accept kernel patches for some mythical future option that > might be happening some time in the future. Greg, these are non mythical "future options" -- they are real feature requests and they have all valid reasons and discussions ongoing on the mailing list. The streaming FPGA support is a concrete valid use case we need to extend *now* and your reply brilliantly seems to have completely ignored it. I do agree that firmware signing itself was a largely debatable topic, there were *really-really* long threads on the topic. However we had a hallway track at Santa Fe Plumbers where it seemed we reached consensus on a path forward! The firmware signing topic should be addressed on TAKASHI's patch set [0]. Please address your NACK and reasons there. Even though it is based on the driver data API, the main topic points of *logic* for it can *also* be discussed there. BUT NOTE: Before we could move forward with firmware signing we need also a *sane* way to extend the API to address the needs of firmware signing. So a proper sane API for future extensions is in order as a prerequisite. This goes along with testing and documentation. This goes for *any* new firmware API feature!!! Firmware signing was just one example feature!!! Also -- even if they were "mythical" the *point* of this series is to address extensibility of the API, and so one of the ways to measure the gains of the design of a new API is to look at what the code would look like when new features are added. This is why I pointed out to review these proposed changes. Even more so given I am noting that these are non-mythical
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > But we don't accept kernel patches for some mythical future option > that might be happening some time in the future. Heck, I'm still not > convinced that firmware signing isn't anything more than just some > snakeoil in the first place! I for one really want the "firmware" signing, because I want to load the regulatory database through this API, and But honestly, I've been waiting for years for that now and started looking at what it would take to hand-implement that on top of the existing firmware API. Probably not all that much. johannes
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote: > But we don't accept kernel patches for some mythical future option > that might be happening some time in the future. Heck, I'm still not > convinced that firmware signing isn't anything more than just some > snakeoil in the first place! I for one really want the "firmware" signing, because I want to load the regulatory database through this API, and But honestly, I've been waiting for years for that now and started looking at what it would take to hand-implement that on top of the existing firmware API. Probably not all that much. johannes
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > As the firmware API evolves we keep extending functions with more > > > arguments. > > > Stop this nonsense by proving an extensible data structure which can be > > > used > > > to represent both user parameters and private internal parameters. > > > > Let's take a simple C function interface and make it a more complex > > data-driven interface that is impossible to understand and obviously > > understand how it is to be used and works! > > The firmware codebase was already complex! Heh, I'm not arguing with you there :) > What you have to ask yourself really is if this makes it *less complex* and > helps *clean things up* in a much better way than it was before. Also does it > allow us to *pave the way for new functionality easily*, without creating > further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. > If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) > > :( > > > > Seriously, why? Why are we extending any of this at all? > > Addressing easy extensibility was a prerequisite of considering firmware > signing > support. Its really the *only* reason I started reviewing the firmware API, > to the > point I started helping fix quite a bit of bugs and now just became the > maintainer. > > So my original firmware signing effort, now driven by AKASHI Takahiro, was one > of the original motivations here! But we don't accept kernel patches for some mythical future option that might be happening some time in the future. Heck, I'm still not convinced that firmware signing isn't anything more than just some snakeoil in the first place! So while you mention lots of times that all sorts of wonderful things can now possibly be built on top of the new code, I have yet to see it (meaning you didn't include it in the patch series.) To get me to take these changes, you have to show a real need and user of the code. Without that it just strongly looks like you are having fun making a more complex api for no reason that we are then going to be stuck with maintaining. So clean away, and fix up, but remember, you have to be able to justify each change as being needed. And so far, I'm not sold on this at all, sorry. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > As the firmware API evolves we keep extending functions with more > > > arguments. > > > Stop this nonsense by proving an extensible data structure which can be > > > used > > > to represent both user parameters and private internal parameters. > > > > Let's take a simple C function interface and make it a more complex > > data-driven interface that is impossible to understand and obviously > > understand how it is to be used and works! > > The firmware codebase was already complex! Heh, I'm not arguing with you there :) > What you have to ask yourself really is if this makes it *less complex* and > helps *clean things up* in a much better way than it was before. Also does it > allow us to *pave the way for new functionality easily*, without creating > further mess? I agree, that's what I'm saying here. I just do not see that happening with your patch set at all. It's adding more code, a more complex way to interact with the subsystem, and not making driver writer lives any easier at all that I can see. Again, the code is now bigger, does more, with not even any real benefit for existing users. > If not, what concrete alternatives do you suggest? It's working, so leave it alone? :) > > :( > > > > Seriously, why? Why are we extending any of this at all? > > Addressing easy extensibility was a prerequisite of considering firmware > signing > support. Its really the *only* reason I started reviewing the firmware API, > to the > point I started helping fix quite a bit of bugs and now just became the > maintainer. > > So my original firmware signing effort, now driven by AKASHI Takahiro, was one > of the original motivations here! But we don't accept kernel patches for some mythical future option that might be happening some time in the future. Heck, I'm still not convinced that firmware signing isn't anything more than just some snakeoil in the first place! So while you mention lots of times that all sorts of wonderful things can now possibly be built on top of the new code, I have yet to see it (meaning you didn't include it in the patch series.) To get me to take these changes, you have to show a real need and user of the code. Without that it just strongly looks like you are having fun making a more complex api for no reason that we are then going to be stuck with maintaining. So clean away, and fix up, but remember, you have to be able to justify each change as being needed. And so far, I'm not sold on this at all, sorry. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/13/2017 2:40 PM, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! The firmware codebase was already complex! What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? If not, what concrete alternatives do you suggest? :( Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! I asked: How we should properly extend the API for new functionality in *a real clean way* ? This also means cleaning under the rug! Ease of extensibility is a need we have as otherwise we end up in the same situation as before, we end up having to add new functionality by extending the number of arguments to an sync or async call, and as collateral have to changes *tons* of callers. I think you and I agree on this, correct me if I'm wrong? Another issue to address when deciding when there is merit for a new API call, instead of just folding functionality into flags. For this its best consider not only the future but also what was done in the past given some new desirable functionality relies on existing mechanisms. The approach Rafał took does what I do just that it forgets about cleaning underneath the rug, so I really cannot see how its any different. Case in point, we have functionality in place today to support no-cache, yet this is a hidden feature, used implicitly only for one API call, there has been requests to support the no-cache functionality by Johannes for iwlwifi long ago for the async call given *they* do their own caching. How do you suggest do we take the no-cache functionality used only internally for another API call and apply it for async? Do we make a new exported symbol ? Or do we expose this as a flag on a regular async call? With the approach I take we expose the internal functionality clearly and later Li Yi folds it as public API through a parameter for the async API. Later he extends iwlwifi with just 2 lines of code to get this functionality. This would later also be used by some other functionality he's interested in! Echo, the firmware caching part is indeed complicated. :) Taking example of the 10 seconds rule of uncaching, bad things could happen if the end drivers could not complete the programming within the time limit or the firmware files was removed from filesystem. It might be safer to let end drivers to free the cache? Anyway it should only be enabled by those drivers need to re-program firmware during suspend/resume instead of as an default option, which is expensive for the base firmware class to do on each PM cycle. I do like the concept of expending features as parameters instead of adding new API functions. Another good example is that the optional call request_firmware_direct() is for sync, but async also has the same need -- do we add a new API call just for the same purpose or do we fold some of this functionality as flags? How do we properly keep evolving functionality for the firmware API? We have to consider existing features, perhaps some hidden, and new features coming down the pipe line. Please take the time to think about all this for a bit, not only the past but also all the incoming set of features in the pipeline! The FPGA streaming support for instance reuses the no-cache "internal hidden feature", as well as the existing request_firmware_into_buf() with some slight variations! We could just fold both requirements as parameters. This series adds a ton of new "features" Actually, it *purposely* only folded the optional nature of firmware for async and added support for enabling drivers to daisy chain requests using a simple api numbering scheme as a counter to complex internal driver mechanisms -- Intel's solution actually used recursion. I split out this work on its own series after a long history of working on firmware singing support. I'm now also cleaning under the rug. I purposely limited the amount of "features" to get proper review. and complexity, The difficulty to
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 6/13/2017 2:40 PM, Luis R. Rodriguez wrote: On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! The firmware codebase was already complex! What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? If not, what concrete alternatives do you suggest? :( Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! I asked: How we should properly extend the API for new functionality in *a real clean way* ? This also means cleaning under the rug! Ease of extensibility is a need we have as otherwise we end up in the same situation as before, we end up having to add new functionality by extending the number of arguments to an sync or async call, and as collateral have to changes *tons* of callers. I think you and I agree on this, correct me if I'm wrong? Another issue to address when deciding when there is merit for a new API call, instead of just folding functionality into flags. For this its best consider not only the future but also what was done in the past given some new desirable functionality relies on existing mechanisms. The approach Rafał took does what I do just that it forgets about cleaning underneath the rug, so I really cannot see how its any different. Case in point, we have functionality in place today to support no-cache, yet this is a hidden feature, used implicitly only for one API call, there has been requests to support the no-cache functionality by Johannes for iwlwifi long ago for the async call given *they* do their own caching. How do you suggest do we take the no-cache functionality used only internally for another API call and apply it for async? Do we make a new exported symbol ? Or do we expose this as a flag on a regular async call? With the approach I take we expose the internal functionality clearly and later Li Yi folds it as public API through a parameter for the async API. Later he extends iwlwifi with just 2 lines of code to get this functionality. This would later also be used by some other functionality he's interested in! Echo, the firmware caching part is indeed complicated. :) Taking example of the 10 seconds rule of uncaching, bad things could happen if the end drivers could not complete the programming within the time limit or the firmware files was removed from filesystem. It might be safer to let end drivers to free the cache? Anyway it should only be enabled by those drivers need to re-program firmware during suspend/resume instead of as an default option, which is expensive for the base firmware class to do on each PM cycle. I do like the concept of expending features as parameters instead of adding new API functions. Another good example is that the optional call request_firmware_direct() is for sync, but async also has the same need -- do we add a new API call just for the same purpose or do we fold some of this functionality as flags? How do we properly keep evolving functionality for the firmware API? We have to consider existing features, perhaps some hidden, and new features coming down the pipe line. Please take the time to think about all this for a bit, not only the past but also all the incoming set of features in the pipeline! The FPGA streaming support for instance reuses the no-cache "internal hidden feature", as well as the existing request_firmware_into_buf() with some slight variations! We could just fold both requirements as parameters. This series adds a ton of new "features" Actually, it *purposely* only folded the optional nature of firmware for async and added support for enabling drivers to daisy chain requests using a simple api numbering scheme as a counter to complex internal driver mechanisms -- Intel's solution actually used recursion. I split out this work on its own series after a long history of working on firmware singing support. I'm now also cleaning under the rug. I purposely limited the amount of "features" to get proper review. and complexity, The difficulty to
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > As the firmware API evolves we keep extending functions with more arguments. > > Stop this nonsense by proving an extensible data structure which can be used > > to represent both user parameters and private internal parameters. > > Let's take a simple C function interface and make it a more complex > data-driven interface that is impossible to understand and obviously > understand how it is to be used and works! The firmware codebase was already complex! What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? If not, what concrete alternatives do you suggest? > :( > > Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! I asked: How we should properly extend the API for new functionality in *a real clean way* ? This also means cleaning under the rug! Ease of extensibility is a need we have as otherwise we end up in the same situation as before, we end up having to add new functionality by extending the number of arguments to an sync or async call, and as collateral have to changes *tons* of callers. I think you and I agree on this, correct me if I'm wrong? Another issue to address when deciding when there is merit for a new API call, instead of just folding functionality into flags. For this its best consider not only the future but also what was done in the past given some new desirable functionality relies on existing mechanisms. The approach Rafał took does what I do just that it forgets about cleaning underneath the rug, so I really cannot see how its any different. Case in point, we have functionality in place today to support no-cache, yet this is a hidden feature, used implicitly only for one API call, there has been requests to support the no-cache functionality by Johannes for iwlwifi long ago for the async call given *they* do their own caching. How do you suggest do we take the no-cache functionality used only internally for another API call and apply it for async? Do we make a new exported symbol ? Or do we expose this as a flag on a regular async call? With the approach I take we expose the internal functionality clearly and later Li Yi folds it as public API through a parameter for the async API. Later he extends iwlwifi with just 2 lines of code to get this functionality. This would later also be used by some other functionality he's interested in! Another good example is that the optional call request_firmware_direct() is for sync, but async also has the same need -- do we add a new API call just for the same purpose or do we fold some of this functionality as flags? How do we properly keep evolving functionality for the firmware API? We have to consider existing features, perhaps some hidden, and new features coming down the pipe line. Please take the time to think about all this for a bit, not only the past but also all the incoming set of features in the pipeline! The FPGA streaming support for instance reuses the no-cache "internal hidden feature", as well as the existing request_firmware_into_buf() with some slight variations! We could just fold both requirements as parameters. > This series adds a ton of new "features" Actually, it *purposely* only folded the optional nature of firmware for async and added support for enabling drivers to daisy chain requests using a simple api numbering scheme as a counter to complex internal driver mechanisms -- Intel's solution actually used recursion. I split out this work on its own series after a long history of working on firmware singing support. I'm now also cleaning under the rug. I purposely limited the amount of "features" to get proper review. > and complexity, The difficulty to properly understand the firmware code *was there before*, so I really do believe its unfair for you to say what I have come up with is complex, question is if its less complex, and *easier* to understand than what we had before. I'd argue it is easier to understand given 2 developers have been working off on top of it now and seem to grok it rather well. Today's code base's complexity is one of the reasons we also we have had quite a bit of regressions. One of the other issues was lack of proper documentation, which I have been fixing as of late, but also clearly documenting the purpose and limitations each new feature. The confusing #ifef'ery over
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > As the firmware API evolves we keep extending functions with more arguments. > > Stop this nonsense by proving an extensible data structure which can be used > > to represent both user parameters and private internal parameters. > > Let's take a simple C function interface and make it a more complex > data-driven interface that is impossible to understand and obviously > understand how it is to be used and works! The firmware codebase was already complex! What you have to ask yourself really is if this makes it *less complex* and helps *clean things up* in a much better way than it was before. Also does it allow us to *pave the way for new functionality easily*, without creating further mess? If not, what concrete alternatives do you suggest? > :( > > Seriously, why? Why are we extending any of this at all? Addressing easy extensibility was a prerequisite of considering firmware signing support. Its really the *only* reason I started reviewing the firmware API, to the point I started helping fix quite a bit of bugs and now just became the maintainer. So my original firmware signing effort, now driven by AKASHI Takahiro, was one of the original motivations here! I asked: How we should properly extend the API for new functionality in *a real clean way* ? This also means cleaning under the rug! Ease of extensibility is a need we have as otherwise we end up in the same situation as before, we end up having to add new functionality by extending the number of arguments to an sync or async call, and as collateral have to changes *tons* of callers. I think you and I agree on this, correct me if I'm wrong? Another issue to address when deciding when there is merit for a new API call, instead of just folding functionality into flags. For this its best consider not only the future but also what was done in the past given some new desirable functionality relies on existing mechanisms. The approach Rafał took does what I do just that it forgets about cleaning underneath the rug, so I really cannot see how its any different. Case in point, we have functionality in place today to support no-cache, yet this is a hidden feature, used implicitly only for one API call, there has been requests to support the no-cache functionality by Johannes for iwlwifi long ago for the async call given *they* do their own caching. How do you suggest do we take the no-cache functionality used only internally for another API call and apply it for async? Do we make a new exported symbol ? Or do we expose this as a flag on a regular async call? With the approach I take we expose the internal functionality clearly and later Li Yi folds it as public API through a parameter for the async API. Later he extends iwlwifi with just 2 lines of code to get this functionality. This would later also be used by some other functionality he's interested in! Another good example is that the optional call request_firmware_direct() is for sync, but async also has the same need -- do we add a new API call just for the same purpose or do we fold some of this functionality as flags? How do we properly keep evolving functionality for the firmware API? We have to consider existing features, perhaps some hidden, and new features coming down the pipe line. Please take the time to think about all this for a bit, not only the past but also all the incoming set of features in the pipeline! The FPGA streaming support for instance reuses the no-cache "internal hidden feature", as well as the existing request_firmware_into_buf() with some slight variations! We could just fold both requirements as parameters. > This series adds a ton of new "features" Actually, it *purposely* only folded the optional nature of firmware for async and added support for enabling drivers to daisy chain requests using a simple api numbering scheme as a counter to complex internal driver mechanisms -- Intel's solution actually used recursion. I split out this work on its own series after a long history of working on firmware singing support. I'm now also cleaning under the rug. I purposely limited the amount of "features" to get proper review. > and complexity, The difficulty to properly understand the firmware code *was there before*, so I really do believe its unfair for you to say what I have come up with is complex, question is if its less complex, and *easier* to understand than what we had before. I'd argue it is easier to understand given 2 developers have been working off on top of it now and seem to grok it rather well. Today's code base's complexity is one of the reasons we also we have had quite a bit of regressions. One of the other issues was lack of proper documentation, which I have been fixing as of late, but also clearly documenting the purpose and limitations each new feature. The confusing #ifef'ery over
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 05:32:49PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote: > > On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > > > On 2017-06-13 11:05, Greg KH wrote: > > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > > As the firmware API evolves we keep extending functions with more > > > > > arguments. > > > > > Stop this nonsense by proving an extensible data structure which can > > > > > be used > > > > > to represent both user parameters and private internal parameters. > > > > > > > > Let's take a simple C function interface and make it a more complex > > > > data-driven interface that is impossible to understand and obviously > > > > understand how it is to be used and works! > > > > > > > > :( > > > > > > > > Seriously, why? Why are we extending any of this at all? This series > > > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > > > > > That's still not worth it at all, you have yet to sell me on this whole > > > > complex beast. I can't see why we need it, and if I, one of the few > > > > people who thinks they actually understand this kernel interface, can't > > > > see it, how can you sell it to someone else? > > > > > > > > Sorry, but no, I'm still not going to take this series until you show > > > > some _REAL_ benefit for it. > > > > > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > > > allow silencing a warning if firmware file is missing. > > > > > > I even sent a trivial patch adding support for this: > > > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > > > https://patchwork.kernel.org/patch/9588787/ > > > (I think it still applies) but it got rejected due to Luis's big rework. > > > > Can you resend this series if it still does apply? > > FWIW just some notes on Rafał's series: > > someone else brought up second that his second no longer should be applied as > some devices do need what seems to be today's optional request. Also note that > the approach follows the same I take, just struct a firmware_opts instead of > driver params... and it does not mesh up the old options as I did in my first > patch in this series. As I have no idea what his series looks like at the moment, why not wait until they are posted again to review them? :) thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 05:32:49PM +0200, Luis R. Rodriguez wrote: > On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote: > > On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > > > On 2017-06-13 11:05, Greg KH wrote: > > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > > As the firmware API evolves we keep extending functions with more > > > > > arguments. > > > > > Stop this nonsense by proving an extensible data structure which can > > > > > be used > > > > > to represent both user parameters and private internal parameters. > > > > > > > > Let's take a simple C function interface and make it a more complex > > > > data-driven interface that is impossible to understand and obviously > > > > understand how it is to be used and works! > > > > > > > > :( > > > > > > > > Seriously, why? Why are we extending any of this at all? This series > > > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > > > > > That's still not worth it at all, you have yet to sell me on this whole > > > > complex beast. I can't see why we need it, and if I, one of the few > > > > people who thinks they actually understand this kernel interface, can't > > > > see it, how can you sell it to someone else? > > > > > > > > Sorry, but no, I'm still not going to take this series until you show > > > > some _REAL_ benefit for it. > > > > > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > > > allow silencing a warning if firmware file is missing. > > > > > > I even sent a trivial patch adding support for this: > > > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > > > https://patchwork.kernel.org/patch/9588787/ > > > (I think it still applies) but it got rejected due to Luis's big rework. > > > > Can you resend this series if it still does apply? > > FWIW just some notes on Rafał's series: > > someone else brought up second that his second no longer should be applied as > some devices do need what seems to be today's optional request. Also note that > the approach follows the same I take, just struct a firmware_opts instead of > driver params... and it does not mesh up the old options as I did in my first > patch in this series. As I have no idea what his series looks like at the moment, why not wait until they are posted again to review them? :) thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote: > On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > > On 2017-06-13 11:05, Greg KH wrote: > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > As the firmware API evolves we keep extending functions with more > > > > arguments. > > > > Stop this nonsense by proving an extensible data structure which can > > > > be used > > > > to represent both user parameters and private internal parameters. > > > > > > Let's take a simple C function interface and make it a more complex > > > data-driven interface that is impossible to understand and obviously > > > understand how it is to be used and works! > > > > > > :( > > > > > > Seriously, why? Why are we extending any of this at all? This series > > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > > > That's still not worth it at all, you have yet to sell me on this whole > > > complex beast. I can't see why we need it, and if I, one of the few > > > people who thinks they actually understand this kernel interface, can't > > > see it, how can you sell it to someone else? > > > > > > Sorry, but no, I'm still not going to take this series until you show > > > some _REAL_ benefit for it. > > > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > > allow silencing a warning if firmware file is missing. > > > > I even sent a trivial patch adding support for this: > > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > > https://patchwork.kernel.org/patch/9588787/ > > (I think it still applies) but it got rejected due to Luis's big rework. > > Can you resend this series if it still does apply? FWIW just some notes on Rafał's series: someone else brought up second that his second no longer should be applied as some devices do need what seems to be today's optional request. Also note that the approach follows the same I take, just struct a firmware_opts instead of driver params... and it does not mesh up the old options as I did in my first patch in this series. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote: > On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > > On 2017-06-13 11:05, Greg KH wrote: > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > As the firmware API evolves we keep extending functions with more > > > > arguments. > > > > Stop this nonsense by proving an extensible data structure which can > > > > be used > > > > to represent both user parameters and private internal parameters. > > > > > > Let's take a simple C function interface and make it a more complex > > > data-driven interface that is impossible to understand and obviously > > > understand how it is to be used and works! > > > > > > :( > > > > > > Seriously, why? Why are we extending any of this at all? This series > > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > > > That's still not worth it at all, you have yet to sell me on this whole > > > complex beast. I can't see why we need it, and if I, one of the few > > > people who thinks they actually understand this kernel interface, can't > > > see it, how can you sell it to someone else? > > > > > > Sorry, but no, I'm still not going to take this series until you show > > > some _REAL_ benefit for it. > > > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > > allow silencing a warning if firmware file is missing. > > > > I even sent a trivial patch adding support for this: > > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > > https://patchwork.kernel.org/patch/9588787/ > > (I think it still applies) but it got rejected due to Luis's big rework. > > Can you resend this series if it still does apply? FWIW just some notes on Rafał's series: someone else brought up second that his second no longer should be applied as some devices do need what seems to be today's optional request. Also note that the approach follows the same I take, just struct a firmware_opts instead of driver params... and it does not mesh up the old options as I did in my first patch in this series. Luis
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 06/13/2017 03:17 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: On 2017-06-13 11:05, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. FWIW I saw (or maybe still see?) a need to extend request_firmware* API to allow silencing a warning if firmware file is missing. I even sent a trivial patch adding support for this: [PATCH V4 1/2] firmware: add more flexible request_firmware_async function https://patchwork.kernel.org/patch/9588787/ (I think it still applies) but it got rejected due to Luis's big rework. Can you resend this series if it still does apply? Sure, if you think it's worth trying, I'll do that! And what exact warning is this silencing? Normally we want the warning there, as that implies that something is wrong if the firmware file that a driver is asking for is not present. That way the user can know to go fix it up, right? It's because brcmfmac looks for NVRAM in two places: /lib/firmware/ and platform NVRAM. It's supposed to silence [ 10.801506] brcmfmac :01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2 in case there is platform NVRAM present. For more details please take a look at: [PATCH V4 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds https://patchwork.kernel.org/patch/9588791/
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 06/13/2017 03:17 PM, Greg KH wrote: On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: On 2017-06-13 11:05, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. FWIW I saw (or maybe still see?) a need to extend request_firmware* API to allow silencing a warning if firmware file is missing. I even sent a trivial patch adding support for this: [PATCH V4 1/2] firmware: add more flexible request_firmware_async function https://patchwork.kernel.org/patch/9588787/ (I think it still applies) but it got rejected due to Luis's big rework. Can you resend this series if it still does apply? Sure, if you think it's worth trying, I'll do that! And what exact warning is this silencing? Normally we want the warning there, as that implies that something is wrong if the firmware file that a driver is asking for is not present. That way the user can know to go fix it up, right? It's because brcmfmac looks for NVRAM in two places: /lib/firmware/ and platform NVRAM. It's supposed to silence [ 10.801506] brcmfmac :01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2 in case there is platform NVRAM present. For more details please take a look at: [PATCH V4 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds https://patchwork.kernel.org/patch/9588791/
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > On 2017-06-13 11:05, Greg KH wrote: > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > As the firmware API evolves we keep extending functions with more > > > arguments. > > > Stop this nonsense by proving an extensible data structure which can > > > be used > > > to represent both user parameters and private internal parameters. > > > > Let's take a simple C function interface and make it a more complex > > data-driven interface that is impossible to understand and obviously > > understand how it is to be used and works! > > > > :( > > > > Seriously, why? Why are we extending any of this at all? This series > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > That's still not worth it at all, you have yet to sell me on this whole > > complex beast. I can't see why we need it, and if I, one of the few > > people who thinks they actually understand this kernel interface, can't > > see it, how can you sell it to someone else? > > > > Sorry, but no, I'm still not going to take this series until you show > > some _REAL_ benefit for it. > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > allow silencing a warning if firmware file is missing. > > I even sent a trivial patch adding support for this: > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > https://patchwork.kernel.org/patch/9588787/ > (I think it still applies) but it got rejected due to Luis's big rework. Can you resend this series if it still does apply? And what exact warning is this silencing? Normally we want the warning there, as that implies that something is wrong if the firmware file that a driver is asking for is not present. That way the user can know to go fix it up, right? thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote: > On 2017-06-13 11:05, Greg KH wrote: > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > As the firmware API evolves we keep extending functions with more > > > arguments. > > > Stop this nonsense by proving an extensible data structure which can > > > be used > > > to represent both user parameters and private internal parameters. > > > > Let's take a simple C function interface and make it a more complex > > data-driven interface that is impossible to understand and obviously > > understand how it is to be used and works! > > > > :( > > > > Seriously, why? Why are we extending any of this at all? This series > > adds a ton of new "features" and complexity, but for absolutely no gain. > > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver. > > > > That's still not worth it at all, you have yet to sell me on this whole > > complex beast. I can't see why we need it, and if I, one of the few > > people who thinks they actually understand this kernel interface, can't > > see it, how can you sell it to someone else? > > > > Sorry, but no, I'm still not going to take this series until you show > > some _REAL_ benefit for it. > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to > allow silencing a warning if firmware file is missing. > > I even sent a trivial patch adding support for this: > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function > https://patchwork.kernel.org/patch/9588787/ > (I think it still applies) but it got rejected due to Luis's big rework. Can you resend this series if it still does apply? And what exact warning is this silencing? Normally we want the warning there, as that implies that something is wrong if the firmware file that a driver is asking for is not present. That way the user can know to go fix it up, right? thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 2017-06-13 11:05, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. FWIW I saw (or maybe still see?) a need to extend request_firmware* API to allow silencing a warning if firmware file is missing. I even sent a trivial patch adding support for this: [PATCH V4 1/2] firmware: add more flexible request_firmware_async function https://patchwork.kernel.org/patch/9588787/ (I think it still applies) but it got rejected due to Luis's big rework. To be honest after seeing this big & more complex driver data API I just gave up and decided I don't care about false problem reports that much :(
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On 2017-06-13 11:05, Greg KH wrote: On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. FWIW I saw (or maybe still see?) a need to extend request_firmware* API to allow silencing a warning if firmware file is missing. I even sent a trivial patch adding support for this: [PATCH V4 1/2] firmware: add more flexible request_firmware_async function https://patchwork.kernel.org/patch/9588787/ (I think it still applies) but it got rejected due to Luis's big rework. To be honest after seeing this big & more complex driver data API I just gave up and decided I don't care about false problem reports that much :(
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > As the firmware API evolves we keep extending functions with more arguments. > Stop this nonsense by proving an extensible data structure which can be used > to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. thanks, greg k-h
Re: [PATCH v9 1/5] firmware: add extensible driver data params
On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > As the firmware API evolves we keep extending functions with more arguments. > Stop this nonsense by proving an extensible data structure which can be used > to represent both user parameters and private internal parameters. Let's take a simple C function interface and make it a more complex data-driven interface that is impossible to understand and obviously understand how it is to be used and works! :( Seriously, why? Why are we extending any of this at all? This series adds a ton of new "features" and complexity, but for absolutely no gain. Oh, I take it back, you removed 29 lines from the iwlwifi driver. That's still not worth it at all, you have yet to sell me on this whole complex beast. I can't see why we need it, and if I, one of the few people who thinks they actually understand this kernel interface, can't see it, how can you sell it to someone else? Sorry, but no, I'm still not going to take this series until you show some _REAL_ benefit for it. thanks, greg k-h
[PATCH v9 1/5] firmware: add extensible driver data params
As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. We introduce 3 data structures: o struct driver_data_req_params - used for user specified parameters o struct driver_data_priv_params - used for internal use only o struct driver_data_params - stiches both of the the above together, only for internal use This starts off by just making the existing APIs use the new data structures, it will make subsequent changes easier to review which will be adding new flexible APIs. A side consequences is get to replace all the old internal "firmware behavior options" flags with enums we properly document, remove the blinding #ifdefs, and compartamentlize the userhelper fallback code more appropriately unde CONFIG_FW_LOADER_USER_HELPER_FALLBACK. This commit should introduces no functional changes (TM). Signed-off-by: Luis R. Rodriguez--- drivers/base/firmware_class.c | 331 -- include/linux/driver_data.h | 89 2 files changed, 346 insertions(+), 74 deletions(-) create mode 100644 include/linux/driver_data.h diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index b9f907eedbf7..db7c0bc0ed98 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -40,6 +41,149 @@ MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); MODULE_LICENSE("GPL"); +/** + * enum driver_data_mode - driver data mode of operation + * @DRIVER_DATA_SYNC: used to determine if we should look for the driver data + * file immediatley. + * @DRIVER_DATA_ASYNC: used to determine if we should schedule the search for + * your driver data file to be run at a later time. + */ +enum driver_data_mode { + DRIVER_DATA_SYNC = 0, + DRIVER_DATA_ASYNC, +}; + +/** + * enum driver_data_priv_reqs - private features only used internally + * + * @DRIVER_DATA_PRIV_REQ_FALLBACK: specifies that the driver data request + * will use a fallback mechanism if the kernel's direct filesystem + * lookup failed to find the requested driver data. If the flag + * %DRIVER_DATA_PRIV_REQ_FALLBACK is set but the flag + * %DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT is not set, it means the caller + * is relying on a custom interface for driver data lookup as a fallback + * mechanism. The custom interface is expected to find any found driver + * data using the exposed sysfs interface of the firmware_class. If the + * custom fallback mechanism is not compatible with the internal caching + * mechanism for driver data lookups at resume, it will be disabled. + * @DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT: indicates that the fallback mechanism + * this driver data request will rely on will be that of having the kernel + * issue a uevent to userspace. Userspace in turn is expected to be + * monitoring for uevents for the firmware_class and will use the + * exposted sysfs interface to upload the driver data for the caller. + * @DRIVER_DATA_PRIV_REQ_NO_CACHE: indicates that the driver data request + * should not set up and use the internal caching mechanism to assist + * drivers from fetching driver data at resume time after suspend. + */ +enum driver_data_priv_reqs { + DRIVER_DATA_PRIV_REQ_FALLBACK = 1 << 0, + DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT= 1 << 1, + DRIVER_DATA_PRIV_REQ_NO_CACHE = 1 << 2, +}; + +/** + * struct driver_data_priv_params - private driver data parameters + * @mode: mode of operation + * @priv_reqs: private set of driver_data_reqs, private requirements for + * the driver data request + * @alloc_buf: buffer area allocated by the caller so we can place the + * respective driver data + * @alloc_buf_size: size of the @alloc_buf + * @old_async_cb: used only for request_firmware_nowait() since we won't change + * all async callbacks to get the return value on failure + */ +struct driver_data_priv_params { + enum driver_data_mode mode; + u64 priv_reqs; + void *alloc_buf; + size_t alloc_buf_size; + void (*old_async_cb)(const struct firmware *driver_data, void *context); +}; + +/** + * struct driver_data_params + * @driver_data: the driver data if found using the requirements specified + * in @req_params and @priv_params + * @req_params: caller's requirements for the driver data to look for + * @priv_params: private requirements for the driver data to look for + */ +struct driver_data_params { + const struct firmware *driver_data; + const struct driver_data_req_params req_params; + struct driver_data_priv_params
[PATCH v9 1/5] firmware: add extensible driver data params
As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters. We introduce 3 data structures: o struct driver_data_req_params - used for user specified parameters o struct driver_data_priv_params - used for internal use only o struct driver_data_params - stiches both of the the above together, only for internal use This starts off by just making the existing APIs use the new data structures, it will make subsequent changes easier to review which will be adding new flexible APIs. A side consequences is get to replace all the old internal "firmware behavior options" flags with enums we properly document, remove the blinding #ifdefs, and compartamentlize the userhelper fallback code more appropriately unde CONFIG_FW_LOADER_USER_HELPER_FALLBACK. This commit should introduces no functional changes (TM). Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_class.c | 331 -- include/linux/driver_data.h | 89 2 files changed, 346 insertions(+), 74 deletions(-) create mode 100644 include/linux/driver_data.h diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index b9f907eedbf7..db7c0bc0ed98 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -40,6 +41,149 @@ MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); MODULE_LICENSE("GPL"); +/** + * enum driver_data_mode - driver data mode of operation + * @DRIVER_DATA_SYNC: used to determine if we should look for the driver data + * file immediatley. + * @DRIVER_DATA_ASYNC: used to determine if we should schedule the search for + * your driver data file to be run at a later time. + */ +enum driver_data_mode { + DRIVER_DATA_SYNC = 0, + DRIVER_DATA_ASYNC, +}; + +/** + * enum driver_data_priv_reqs - private features only used internally + * + * @DRIVER_DATA_PRIV_REQ_FALLBACK: specifies that the driver data request + * will use a fallback mechanism if the kernel's direct filesystem + * lookup failed to find the requested driver data. If the flag + * %DRIVER_DATA_PRIV_REQ_FALLBACK is set but the flag + * %DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT is not set, it means the caller + * is relying on a custom interface for driver data lookup as a fallback + * mechanism. The custom interface is expected to find any found driver + * data using the exposed sysfs interface of the firmware_class. If the + * custom fallback mechanism is not compatible with the internal caching + * mechanism for driver data lookups at resume, it will be disabled. + * @DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT: indicates that the fallback mechanism + * this driver data request will rely on will be that of having the kernel + * issue a uevent to userspace. Userspace in turn is expected to be + * monitoring for uevents for the firmware_class and will use the + * exposted sysfs interface to upload the driver data for the caller. + * @DRIVER_DATA_PRIV_REQ_NO_CACHE: indicates that the driver data request + * should not set up and use the internal caching mechanism to assist + * drivers from fetching driver data at resume time after suspend. + */ +enum driver_data_priv_reqs { + DRIVER_DATA_PRIV_REQ_FALLBACK = 1 << 0, + DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT= 1 << 1, + DRIVER_DATA_PRIV_REQ_NO_CACHE = 1 << 2, +}; + +/** + * struct driver_data_priv_params - private driver data parameters + * @mode: mode of operation + * @priv_reqs: private set of driver_data_reqs, private requirements for + * the driver data request + * @alloc_buf: buffer area allocated by the caller so we can place the + * respective driver data + * @alloc_buf_size: size of the @alloc_buf + * @old_async_cb: used only for request_firmware_nowait() since we won't change + * all async callbacks to get the return value on failure + */ +struct driver_data_priv_params { + enum driver_data_mode mode; + u64 priv_reqs; + void *alloc_buf; + size_t alloc_buf_size; + void (*old_async_cb)(const struct firmware *driver_data, void *context); +}; + +/** + * struct driver_data_params + * @driver_data: the driver data if found using the requirements specified + * in @req_params and @priv_params + * @req_params: caller's requirements for the driver data to look for + * @priv_params: private requirements for the driver data to look for + */ +struct driver_data_params { + const struct firmware *driver_data; + const struct driver_data_req_params req_params; + struct driver_data_priv_params priv_params; +}; +