From: Robin Murphy <robin.murphy@arm.com> To: Paul Gortmaker <paul.gortmaker@windriver.com> Cc: Joerg Roedel <joro@8bytes.org>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Will Deacon <will.deacon@arm.com>, Nate Watterson <nwatters@codeaurora.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular Date: Wed, 28 Nov 2018 17:28:16 +0000 [thread overview] Message-ID: <d952cb5e-b524-5eea-1729-4d579f637152@arm.com> (raw) In-Reply-To: <20181128152445.GH14659@windriver.com> On 28/11/2018 15:24, Paul Gortmaker wrote: > [Re: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular] On 28/11/2018 (Wed 12:42) Robin Murphy wrote: > >> Hi Paul, >> >> On 26/11/2018 22:31, Paul Gortmaker wrote: > > [...] > >>> We add a moduleparam.h include since the file does actually declare >>> some module parameters, and leaving them as such is the easiest way >>> currently to remain backwards compatible with existing use cases. > > [...] > > >> Is it worth introducing builtin_param() and friends for this sort of thing, >> to echo the *_platform_driver() helpers? It seems like that could be >> justifiable under the motivation described in the cover letter. > > I've definitely gone back and looked at this a few times when coming > across the few corner cases like these, to remind myself why I didn't do > it already. > > We'd not want to replicate all the module_param stuff as an instance of > builtin_param() because we already have setup() and setup_param() in > init.h -- however they don't do the file name in the param - hence the > reason it isn't a direct swap in replacement. > > So, it would become some more complex refactoring of moduleparam.h into > say bootparam.h - to reduce code/macro duplication, while at the same > time being aware of existing setup_param stuff and making something like > a new setup_param_named() that is consistent with existing setup fcns. > > And based on past experience, there will be reviewers who don't see the > value in the distinction and simply reply with two words "why bother?". > > Not impossible, but not as simple as the builtin_platform_driver and > similar wrappers that I've already added to mainline. You've made we > want to go have another look at it again, but in the meantime we can do > what I've done here, and circle around later to update the few instances > of moduleparam in non-modules once/if the refactoring I describe above > works out and is accepted in mainline. Sure, I definitely agree with doing a first pass like this to sweep up all the cruft and audit the module_param users at the same time, then considering a robust refactoring once we've got a clear idea of how many users actually need it. TBH, at this point I was thinking along the lines of a simple: #ifndef MODULE #define builtin_param(name, type, perm) \ module_param(name, type, perm) #define builtin_param_named(name, name, type, perrm) \ module_param_named(name, name, type, perm) #endif still in moduleparam.h, purely so that the intent can be made really clear in driver code and it's more searchable than just comments. But yeah, even that would probably be objectionable to many. >> Otherwise, the changes look reasonable. I still hold out hope that one day >> we'll be able to make IOMMU drivers modular (it can work with minimal hacks >> today, but it's far from robust in general), but for now I agree this makes >> sense (and it'll be easy enough to revert for playing with further hacks). > > I totally agree - I've had similar discussions with the DMA maintainers, > and if being modular can be made to work and has a use case - great! > > But it should be a conscious decision, since nobody writes a new driver > from scratch; they copy one that is "close" as a template and then go > from there. Which leads to a good percentage of drivers having hints > of modular stuff when there is no intent of them ever being modular. > >> With the title fixed up as Joerg asked, >> >> Acked-by: Robin Murphy <robin.murphy@arm.com> > > Thanks for the feedback/review. Will re-send with updated titles before > the week is finished and a good chance for additional feedback has elapsed. Great! There's probably some more subtleties that could be tidied up when the driver is truly non-removable, but I can take a look at that myself once this patch has gone in. Cheers, Robin.
WARNING: multiple messages have this Message-ID (diff)
From: robin.murphy@arm.com (Robin Murphy) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular Date: Wed, 28 Nov 2018 17:28:16 +0000 [thread overview] Message-ID: <d952cb5e-b524-5eea-1729-4d579f637152@arm.com> (raw) In-Reply-To: <20181128152445.GH14659@windriver.com> On 28/11/2018 15:24, Paul Gortmaker wrote: > [Re: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular] On 28/11/2018 (Wed 12:42) Robin Murphy wrote: > >> Hi Paul, >> >> On 26/11/2018 22:31, Paul Gortmaker wrote: > > [...] > >>> We add a moduleparam.h include since the file does actually declare >>> some module parameters, and leaving them as such is the easiest way >>> currently to remain backwards compatible with existing use cases. > > [...] > > >> Is it worth introducing builtin_param() and friends for this sort of thing, >> to echo the *_platform_driver() helpers? It seems like that could be >> justifiable under the motivation described in the cover letter. > > I've definitely gone back and looked at this a few times when coming > across the few corner cases like these, to remind myself why I didn't do > it already. > > We'd not want to replicate all the module_param stuff as an instance of > builtin_param() because we already have setup() and setup_param() in > init.h -- however they don't do the file name in the param - hence the > reason it isn't a direct swap in replacement. > > So, it would become some more complex refactoring of moduleparam.h into > say bootparam.h - to reduce code/macro duplication, while at the same > time being aware of existing setup_param stuff and making something like > a new setup_param_named() that is consistent with existing setup fcns. > > And based on past experience, there will be reviewers who don't see the > value in the distinction and simply reply with two words "why bother?". > > Not impossible, but not as simple as the builtin_platform_driver and > similar wrappers that I've already added to mainline. You've made we > want to go have another look at it again, but in the meantime we can do > what I've done here, and circle around later to update the few instances > of moduleparam in non-modules once/if the refactoring I describe above > works out and is accepted in mainline. Sure, I definitely agree with doing a first pass like this to sweep up all the cruft and audit the module_param users at the same time, then considering a robust refactoring once we've got a clear idea of how many users actually need it. TBH, at this point I was thinking along the lines of a simple: #ifndef MODULE #define builtin_param(name, type, perm) \ module_param(name, type, perm) #define builtin_param_named(name, name, type, perrm) \ module_param_named(name, name, type, perm) #endif still in moduleparam.h, purely so that the intent can be made really clear in driver code and it's more searchable than just comments. But yeah, even that would probably be objectionable to many. >> Otherwise, the changes look reasonable. I still hold out hope that one day >> we'll be able to make IOMMU drivers modular (it can work with minimal hacks >> today, but it's far from robust in general), but for now I agree this makes >> sense (and it'll be easy enough to revert for playing with further hacks). > > I totally agree - I've had similar discussions with the DMA maintainers, > and if being modular can be made to work and has a use case - great! > > But it should be a conscious decision, since nobody writes a new driver > from scratch; they copy one that is "close" as a template and then go > from there. Which leads to a good percentage of drivers having hints > of modular stuff when there is no intent of them ever being modular. > >> With the title fixed up as Joerg asked, >> >> Acked-by: Robin Murphy <robin.murphy@arm.com> > > Thanks for the feedback/review. Will re-send with updated titles before > the week is finished and a good chance for additional feedback has elapsed. Great! There's probably some more subtleties that could be tidied up when the driver is truly non-removable, but I can take a look at that myself once this patch has gone in. Cheers, Robin.
next prev parent reply other threads:[~2018-11-28 17:28 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-26 22:31 [PATCH 0/9] iommu: clean up/remove modular stuff from non-modules Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` [PATCH 1/9] iommu: audit and remove any unnecessary uses of module.h Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` [PATCH 2/9] iommu: rockchip: make it explicitly non-modular Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 23:37 ` Heiko Stuebner 2018-11-26 23:37 ` Heiko Stuebner 2018-11-26 23:37 ` Heiko Stuebner 2018-11-26 22:31 ` [PATCH 3/9] iommu: msm_iommu: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` [PATCH 4/9] iommu: mtk_iommu: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-27 1:25 ` Honghui Zhang 2018-11-27 1:25 ` Honghui Zhang 2018-11-26 22:31 ` [PATCH 5/9] iommu: ipmmu-vmsa: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-28 12:50 ` Robin Murphy 2018-11-28 15:32 ` Paul Gortmaker 2018-11-28 15:32 ` Paul Gortmaker 2018-11-28 17:22 ` Laurent Pinchart 2018-11-28 22:10 ` Paul Gortmaker 2018-11-28 22:10 ` Paul Gortmaker [not found] ` <1543271498-28966-1-git-send-email-paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org> 2018-11-26 22:31 ` [PATCH 6/9] iommu: qcom_iommu: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` [PATCH 7/9] iommu: tegra-gart: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-27 14:18 ` Thierry Reding 2018-11-26 22:31 ` [PATCH 8/9] iommu: arm-smmu: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-28 12:42 ` Robin Murphy 2018-11-28 12:42 ` Robin Murphy 2018-11-28 15:24 ` Paul Gortmaker 2018-11-28 15:24 ` Paul Gortmaker 2018-11-28 15:24 ` Paul Gortmaker 2018-11-28 17:28 ` Robin Murphy [this message] 2018-11-28 17:28 ` Robin Murphy 2018-11-26 22:31 ` [PATCH 9/9] iommu: arm-smmu-v3: " Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-26 22:31 ` Paul Gortmaker 2018-11-28 12:44 ` Robin Murphy 2018-11-28 12:44 ` Robin Murphy 2018-11-28 12:44 ` Robin Murphy 2018-11-27 10:11 ` [PATCH 0/9] iommu: clean up/remove modular stuff from non-modules Joerg Roedel 2018-11-27 10:11 ` Joerg Roedel 2018-11-27 10:11 ` Joerg Roedel [not found] ` <20181127101156.GA12298-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> 2018-11-27 14:39 ` Paul Gortmaker 2018-11-27 14:39 ` Paul Gortmaker 2018-11-27 14:39 ` Paul Gortmaker
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d952cb5e-b524-5eea-1729-4d579f637152@arm.com \ --to=robin.murphy@arm.com \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nwatters@codeaurora.org \ --cc=paul.gortmaker@windriver.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.