To provide a new IIO trigger to the IIO core, usually driver executes the
following pipeline: allocate()/register()/get(). Before, IIO core assigned
trig->owner as a pointer to the module which registered this trigger at
the register() stage. But actually the trigger object is owned by the
module earlier, on the allocate() stage, when trigger object is
successfully allocated for the driver.
This patch moves trig->owner initialization from register()
stage of trigger initialization pipeline to allocate() stage to
eliminate all misunderstandings and time gaps between trigger object
creation and owner acquiring.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
drivers/iio/industrialio-trigger.c | 46 ++++++++++++++++--------------
include/linux/iio/iio.h | 10 +++++--
include/linux/iio/trigger.h | 21 +++++++-------
3 files changed, 42 insertions(+), 35 deletions(-)
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index f504ed351b3e..143c8f360499 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -64,13 +64,10 @@ ATTRIBUTE_GROUPS(iio_trig_dev);
static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
-int __iio_trigger_register(struct iio_trigger *trig_info,
- struct module *this_mod)
+int iio_trigger_register(struct iio_trigger *trig_info)
{
int ret;
- trig_info->owner = this_mod;
-
trig_info->id = ida_simple_get(&iio_trigger_ida, 0, 0, GFP_KERNEL);
if (trig_info->id < 0)
return trig_info->id;
@@ -101,7 +98,7 @@ int __iio_trigger_register(struct iio_trigger *trig_info,
ida_simple_remove(&iio_trigger_ida, trig_info->id);
return ret;
}
-EXPORT_SYMBOL(__iio_trigger_register);
+EXPORT_SYMBOL(iio_trigger_register);
void iio_trigger_unregister(struct iio_trigger *trig_info)
{
@@ -552,8 +549,9 @@ static void iio_trig_subirqunmask(struct irq_data *d)
trig->subirqs[d->irq - trig->subirq_base].enabled = true;
}
-static __printf(2, 0)
+static __printf(3, 0)
struct iio_trigger *viio_trigger_alloc(struct device *parent,
+ struct module *this_mod,
const char *fmt,
va_list vargs)
{
@@ -581,6 +579,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
if (trig->name == NULL)
goto free_descs;
+ trig->owner = this_mod;
+
trig->subirq_chip.name = trig->name;
trig->subirq_chip.irq_mask = &iio_trig_subirqmask;
trig->subirq_chip.irq_unmask = &iio_trig_subirqunmask;
@@ -601,8 +601,9 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
}
/**
- * iio_trigger_alloc - Allocate a trigger
+ * __iio_trigger_alloc - Allocate a trigger
* @parent: Device to allocate iio_trigger for
+ * @this_mod: module allocating the trigger
* @fmt: trigger name format. If it includes format
* specifiers, the additional arguments following
* format are formatted and inserted in the resulting
@@ -610,18 +611,20 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
* RETURNS:
* Pointer to allocated iio_trigger on success, NULL on failure.
*/
-struct iio_trigger *iio_trigger_alloc(struct device *parent, const char *fmt, ...)
+struct iio_trigger *__iio_trigger_alloc(struct device *parent,
+ struct module *this_mod,
+ const char *fmt, ...)
{
struct iio_trigger *trig;
va_list vargs;
va_start(vargs, fmt);
- trig = viio_trigger_alloc(parent, fmt, vargs);
+ trig = viio_trigger_alloc(parent, this_mod, fmt, vargs);
va_end(vargs);
return trig;
}
-EXPORT_SYMBOL(iio_trigger_alloc);
+EXPORT_SYMBOL(__iio_trigger_alloc);
void iio_trigger_free(struct iio_trigger *trig)
{
@@ -636,10 +639,11 @@ static void devm_iio_trigger_release(struct device *dev, void *res)
}
/**
- * devm_iio_trigger_alloc - Resource-managed iio_trigger_alloc()
+ * __devm_iio_trigger_alloc - Resource-managed iio_trigger_alloc()
* Managed iio_trigger_alloc. iio_trigger allocated with this function is
* automatically freed on driver detach.
* @parent: Device to allocate iio_trigger for
+ * @this_mod: module allocating the trigger
* @fmt: trigger name format. If it includes format
* specifiers, the additional arguments following
* format are formatted and inserted in the resulting
@@ -649,7 +653,9 @@ static void devm_iio_trigger_release(struct device *dev, void *res)
* RETURNS:
* Pointer to allocated iio_trigger on success, NULL on failure.
*/
-struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, const char *fmt, ...)
+struct iio_trigger *__devm_iio_trigger_alloc(struct device *parent,
+ struct module *this_mod,
+ const char *fmt, ...)
{
struct iio_trigger **ptr, *trig;
va_list vargs;
@@ -661,7 +667,7 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, const char *fm
/* use raw alloc_dr for kmalloc caller tracing */
va_start(vargs, fmt);
- trig = viio_trigger_alloc(parent, fmt, vargs);
+ trig = viio_trigger_alloc(parent, this_mod, fmt, vargs);
va_end(vargs);
if (trig) {
*ptr = trig;
@@ -672,7 +678,7 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, const char *fm
return trig;
}
-EXPORT_SYMBOL_GPL(devm_iio_trigger_alloc);
+EXPORT_SYMBOL_GPL(__devm_iio_trigger_alloc);
static void devm_iio_trigger_unreg(void *trigger_info)
{
@@ -680,10 +686,9 @@ static void devm_iio_trigger_unreg(void *trigger_info)
}
/**
- * __devm_iio_trigger_register - Resource-managed iio_trigger_register()
+ * devm_iio_trigger_register - Resource-managed iio_trigger_register()
* @dev: device this trigger was allocated for
* @trig_info: trigger to register
- * @this_mod: module registering the trigger
*
* Managed iio_trigger_register(). The IIO trigger registered with this
* function is automatically unregistered on driver detach. This function
@@ -693,19 +698,18 @@ static void devm_iio_trigger_unreg(void *trigger_info)
* RETURNS:
* 0 on success, negative error number on failure.
*/
-int __devm_iio_trigger_register(struct device *dev,
- struct iio_trigger *trig_info,
- struct module *this_mod)
+int devm_iio_trigger_register(struct device *dev,
+ struct iio_trigger *trig_info)
{
int ret;
- ret = __iio_trigger_register(trig_info, this_mod);
+ ret = iio_trigger_register(trig_info);
if (ret)
return ret;
return devm_add_action_or_reset(dev, devm_iio_trigger_unreg, trig_info);
}
-EXPORT_SYMBOL_GPL(__devm_iio_trigger_register);
+EXPORT_SYMBOL_GPL(devm_iio_trigger_register);
bool iio_trigger_using_own(struct iio_dev *indio_dev)
{
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 07025d6b3de1..853daa191b3c 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -669,9 +669,13 @@ static inline void *iio_priv(const struct iio_dev *indio_dev)
void iio_device_free(struct iio_dev *indio_dev);
struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
-__printf(2, 3)
-struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
- const char *fmt, ...);
+
+#define devm_iio_trigger_alloc(parent, fmt, ...) \
+ __devm_iio_trigger_alloc((parent), THIS_MODULE, (fmt), ##__VA_ARGS__)
+__printf(3, 4)
+struct iio_trigger *__devm_iio_trigger_alloc(struct device *parent,
+ struct module *this_mod,
+ const char *fmt, ...);
/**
* iio_buffer_enabled() - helper function to test if the buffer is enabled
* @indio_dev: IIO device structure for device
diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index 4a008b952710..5abec8df5a18 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -135,16 +135,10 @@ static inline void *iio_trigger_get_drvdata(struct iio_trigger *trig)
* iio_trigger_register() - register a trigger with the IIO core
* @trig_info: trigger to be registered
**/
-#define iio_trigger_register(trig_info) \
- __iio_trigger_register((trig_info), THIS_MODULE)
-int __iio_trigger_register(struct iio_trigger *trig_info,
- struct module *this_mod);
+int iio_trigger_register(struct iio_trigger *trig_info);
-#define devm_iio_trigger_register(dev, trig_info) \
- __devm_iio_trigger_register((dev), (trig_info), THIS_MODULE)
-int __devm_iio_trigger_register(struct device *dev,
- struct iio_trigger *trig_info,
- struct module *this_mod);
+int devm_iio_trigger_register(struct device *dev,
+ struct iio_trigger *trig_info);
/**
* iio_trigger_unregister() - unregister a trigger from the core
@@ -172,8 +166,13 @@ void iio_trigger_poll_chained(struct iio_trigger *trig);
irqreturn_t iio_trigger_generic_data_rdy_poll(int irq, void *private);
-__printf(2, 3)
-struct iio_trigger *iio_trigger_alloc(struct device *parent, const char *fmt, ...);
+#define iio_trigger_alloc(parent, fmt, ...) \
+ __iio_trigger_alloc((parent), THIS_MODULE, (fmt), ##__VA_ARGS__)
+
+__printf(3, 4)
+struct iio_trigger *__iio_trigger_alloc(struct device *parent,
+ struct module *this_mod,
+ const char *fmt, ...);
void iio_trigger_free(struct iio_trigger *trig);
/**
--
2.36.0
On Wed, 1 Jun 2022 17:48:32 +0000
Dmitry Rokosov <[email protected]> wrote:
> To provide a new IIO trigger to the IIO core, usually driver executes the
> following pipeline: allocate()/register()/get(). Before, IIO core assigned
> trig->owner as a pointer to the module which registered this trigger at
> the register() stage. But actually the trigger object is owned by the
> module earlier, on the allocate() stage, when trigger object is
> successfully allocated for the driver.
>
> This patch moves trig->owner initialization from register()
> stage of trigger initialization pipeline to allocate() stage to
> eliminate all misunderstandings and time gaps between trigger object
> creation and owner acquiring.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
Hi Dmitry,
I 'think' this is fine, but its in the high risk category that I'd like
to keep it on list for a few weeks before applying.
Note I'm still keen that in general we keep the flow such that
we do allocate()/register()/get() as there is no guarantee that the get()
will never do anything that requires the trigger to be registered, even
though that is true today. Which is another way of saying I'm still
keen we fix up any cases that sneak in after your fix up set dealt with
the current ones.
Thanks for following up on this!
Jonathan
Hello Jonathan,
Thank you for comments. I have a several questions about the flow,
please find them below.
On Sat, Jun 04, 2022 at 02:59:55PM +0100, Jonathan Cameron wrote:
> On Wed, 1 Jun 2022 17:48:32 +0000
> Dmitry Rokosov <[email protected]> wrote:
>
> > To provide a new IIO trigger to the IIO core, usually driver executes the
> > following pipeline: allocate()/register()/get(). Before, IIO core assigned
> > trig->owner as a pointer to the module which registered this trigger at
> > the register() stage. But actually the trigger object is owned by the
> > module earlier, on the allocate() stage, when trigger object is
> > successfully allocated for the driver.
> >
> > This patch moves trig->owner initialization from register()
> > stage of trigger initialization pipeline to allocate() stage to
> > eliminate all misunderstandings and time gaps between trigger object
> > creation and owner acquiring.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
>
> Hi Dmitry,
>
> I 'think' this is fine, but its in the high risk category that I'd like
> to keep it on list for a few weeks before applying.
>
Could you please explain what it means? Do you have some testing branch
with such dangerous patches or do we need just to wait other developers
for more points of view? Thanks in advance.
> Note I'm still keen that in general we keep the flow such that
> we do allocate()/register()/get() as there is no guarantee that the get()
> will never do anything that requires the trigger to be registered, even
> though that is true today. Which is another way of saying I'm still
> keen we fix up any cases that sneak in after your fix up set dealt with
> the current ones.
I fully agree with you. I suppose to resolve such a problem we need to
have some indicators that the trigger is already registered or not.
From my point of view, trig->list entry fits well to answer this question.
Trigger is added to the global IIO triggers list during register()
execution, so we can just check that entry is not empty to make sure that
trigger is registered.
I've sent a v2 patch version, where I use trig->list entry empty status to
warn it:
https://lore.kernel.org/linux-iio/[email protected]/
>
> Thanks for following up on this!
>
> Jonathan
>
--
Thank you,
Dmitry
Hello Jonathan,
This patch has been on the mailing list for one month already, but no
comments from other IIO reviewers. What do you think we should do with it?
Is it a helpful change or not?
On Sat, Jun 04, 2022 at 02:59:55PM +0100, Jonathan Cameron wrote:
> On Wed, 1 Jun 2022 17:48:32 +0000
> Dmitry Rokosov <[email protected]> wrote:
>
> > To provide a new IIO trigger to the IIO core, usually driver executes the
> > following pipeline: allocate()/register()/get(). Before, IIO core assigned
> > trig->owner as a pointer to the module which registered this trigger at
> > the register() stage. But actually the trigger object is owned by the
> > module earlier, on the allocate() stage, when trigger object is
> > successfully allocated for the driver.
> >
> > This patch moves trig->owner initialization from register()
> > stage of trigger initialization pipeline to allocate() stage to
> > eliminate all misunderstandings and time gaps between trigger object
> > creation and owner acquiring.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
>
> Hi Dmitry,
>
> I 'think' this is fine, but its in the high risk category that I'd like
> to keep it on list for a few weeks before applying.
>
> Note I'm still keen that in general we keep the flow such that
> we do allocate()/register()/get() as there is no guarantee that the get()
> will never do anything that requires the trigger to be registered, even
> though that is true today. Which is another way of saying I'm still
> keen we fix up any cases that sneak in after your fix up set dealt with
> the current ones.
>
> Thanks for following up on this!
>
> Jonathan
>
--
Thank you,
Dmitry
On Fri, 1 Jul 2022 11:59:59 +0000
Dmitry Rokosov <[email protected]> wrote:
> Hello Jonathan,
>
> This patch has been on the mailing list for one month already, but no
> comments from other IIO reviewers. What do you think we should do with it?
> Is it a helpful change or not?
Given I'm way behind and timing in cycle, I'm probably going to kick this
back to start of the next cycle. Sorry for delay,
Jonathan
>
> On Sat, Jun 04, 2022 at 02:59:55PM +0100, Jonathan Cameron wrote:
> > On Wed, 1 Jun 2022 17:48:32 +0000
> > Dmitry Rokosov <[email protected]> wrote:
> >
> > > To provide a new IIO trigger to the IIO core, usually driver executes the
> > > following pipeline: allocate()/register()/get(). Before, IIO core assigned
> > > trig->owner as a pointer to the module which registered this trigger at
> > > the register() stage. But actually the trigger object is owned by the
> > > module earlier, on the allocate() stage, when trigger object is
> > > successfully allocated for the driver.
> > >
> > > This patch moves trig->owner initialization from register()
> > > stage of trigger initialization pipeline to allocate() stage to
> > > eliminate all misunderstandings and time gaps between trigger object
> > > creation and owner acquiring.
> > >
> > > Signed-off-by: Dmitry Rokosov <[email protected]>
> >
> > Hi Dmitry,
> >
> > I 'think' this is fine, but its in the high risk category that I'd like
> > to keep it on list for a few weeks before applying.
> >
> > Note I'm still keen that in general we keep the flow such that
> > we do allocate()/register()/get() as there is no guarantee that the get()
> > will never do anything that requires the trigger to be registered, even
> > though that is true today. Which is another way of saying I'm still
> > keen we fix up any cases that sneak in after your fix up set dealt with
> > the current ones.
> >
> > Thanks for following up on this!
> >
> > Jonathan
> >
>
On Wed, 13 Jul 2022 17:04:36 +0100
Jonathan Cameron <[email protected]> wrote:
> On Fri, 1 Jul 2022 11:59:59 +0000
> Dmitry Rokosov <[email protected]> wrote:
>
> > Hello Jonathan,
> >
> > This patch has been on the mailing list for one month already, but no
> > comments from other IIO reviewers. What do you think we should do with it?
> > Is it a helpful change or not?
>
> Given I'm way behind and timing in cycle, I'm probably going to kick this
> back to start of the next cycle. Sorry for delay,
Applied to the togreg branch of iio.git.
I'm unlikely to do another pull request this cycle unless there is a delay in
the release for some reason (and probably not even if there is), so this
is queued up for next cycle. As such it'll sit exposed only in the testing
branch until I rebase on rc1.
Thanks,
Jonathan
>
> Jonathan
>
> >
> > On Sat, Jun 04, 2022 at 02:59:55PM +0100, Jonathan Cameron wrote:
> > > On Wed, 1 Jun 2022 17:48:32 +0000
> > > Dmitry Rokosov <[email protected]> wrote:
> > >
> > > > To provide a new IIO trigger to the IIO core, usually driver executes the
> > > > following pipeline: allocate()/register()/get(). Before, IIO core assigned
> > > > trig->owner as a pointer to the module which registered this trigger at
> > > > the register() stage. But actually the trigger object is owned by the
> > > > module earlier, on the allocate() stage, when trigger object is
> > > > successfully allocated for the driver.
> > > >
> > > > This patch moves trig->owner initialization from register()
> > > > stage of trigger initialization pipeline to allocate() stage to
> > > > eliminate all misunderstandings and time gaps between trigger object
> > > > creation and owner acquiring.
> > > >
> > > > Signed-off-by: Dmitry Rokosov <[email protected]>
> > >
> > > Hi Dmitry,
> > >
> > > I 'think' this is fine, but its in the high risk category that I'd like
> > > to keep it on list for a few weeks before applying.
> > >
> > > Note I'm still keen that in general we keep the flow such that
> > > we do allocate()/register()/get() as there is no guarantee that the get()
> > > will never do anything that requires the trigger to be registered, even
> > > though that is true today. Which is another way of saying I'm still
> > > keen we fix up any cases that sneak in after your fix up set dealt with
> > > the current ones.
> > >
> > > Thanks for following up on this!
> > >
> > > Jonathan
> > >
> >
>
Hello Jonathan,
On Sat, Jul 16, 2022 at 04:25:56PM +0100, Jonathan Cameron wrote:
> On Wed, 13 Jul 2022 17:04:36 +0100
> Jonathan Cameron <[email protected]> wrote:
>
> > On Fri, 1 Jul 2022 11:59:59 +0000
> > Dmitry Rokosov <[email protected]> wrote:
> >
> > > Hello Jonathan,
> > >
> > > This patch has been on the mailing list for one month already, but no
> > > comments from other IIO reviewers. What do you think we should do with it?
> > > Is it a helpful change or not?
> >
> > Given I'm way behind and timing in cycle, I'm probably going to kick this
> > back to start of the next cycle. Sorry for delay,
> Applied to the togreg branch of iio.git.
>
> I'm unlikely to do another pull request this cycle unless there is a delay in
> the release for some reason (and probably not even if there is), so this
> is queued up for next cycle. As such it'll sit exposed only in the testing
> branch until I rebase on rc1.
>
> Thanks,
>
> Jonathan
>
Thank you for the patch applied.
I have one question about the previous already applied patchset
https://lore.kernel.org/all/[email protected]/
I see this patchset already merged to linux-next more than a month ago.
But it's still not available in the linux stable branch. Could you please
explain what's the problem with this one? Was some bug found during
linux-next testing stage? Should I fix something?
Appreciate any help to understand what's I missing.
--
Thank you,
Dmitry
On Mon, 18 Jul 2022 11:23:59 +0000
Dmitry Rokosov <[email protected]> wrote:
> Hello Jonathan,
>
> On Sat, Jul 16, 2022 at 04:25:56PM +0100, Jonathan Cameron wrote:
> > On Wed, 13 Jul 2022 17:04:36 +0100
> > Jonathan Cameron <[email protected]> wrote:
> >
> > > On Fri, 1 Jul 2022 11:59:59 +0000
> > > Dmitry Rokosov <[email protected]> wrote:
> > >
> > > > Hello Jonathan,
> > > >
> > > > This patch has been on the mailing list for one month already, but no
> > > > comments from other IIO reviewers. What do you think we should do with it?
> > > > Is it a helpful change or not?
> > >
> > > Given I'm way behind and timing in cycle, I'm probably going to kick this
> > > back to start of the next cycle. Sorry for delay,
> > Applied to the togreg branch of iio.git.
> >
> > I'm unlikely to do another pull request this cycle unless there is a delay in
> > the release for some reason (and probably not even if there is), so this
> > is queued up for next cycle. As such it'll sit exposed only in the testing
> > branch until I rebase on rc1.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Thank you for the patch applied.
> I have one question about the previous already applied patchset
>
> https://lore.kernel.org/all/[email protected]/
>
> I see this patchset already merged to linux-next more than a month ago.
> But it's still not available in the linux stable branch. Could you please
> explain what's the problem with this one? Was some bug found during
> linux-next testing stage? Should I fix something?
>
> Appreciate any help to understand what's I missing.
It's not a fix so it will go in during the merge window in about 2 weeks time.
Won't get backported to Stable though unless we ask for that to happen as it's
not really a fix so I didn't add a marking for it to be picked up for stable
(which would only happen after 5.20-rc1 anyway).
J
>
On Mon, Jul 18, 2022 at 06:32:49PM +0100, Jonathan Cameron wrote:
> On Mon, 18 Jul 2022 11:23:59 +0000
> Dmitry Rokosov <[email protected]> wrote:
>
> > Hello Jonathan,
> >
> > On Sat, Jul 16, 2022 at 04:25:56PM +0100, Jonathan Cameron wrote:
> > > On Wed, 13 Jul 2022 17:04:36 +0100
> > > Jonathan Cameron <[email protected]> wrote:
> > >
> > > > On Fri, 1 Jul 2022 11:59:59 +0000
> > > > Dmitry Rokosov <[email protected]> wrote:
> > > >
> > > > > Hello Jonathan,
> > > > >
> > > > > This patch has been on the mailing list for one month already, but no
> > > > > comments from other IIO reviewers. What do you think we should do with it?
> > > > > Is it a helpful change or not?
> > > >
> > > > Given I'm way behind and timing in cycle, I'm probably going to kick this
> > > > back to start of the next cycle. Sorry for delay,
> > > Applied to the togreg branch of iio.git.
> > >
> > > I'm unlikely to do another pull request this cycle unless there is a delay in
> > > the release for some reason (and probably not even if there is), so this
> > > is queued up for next cycle. As such it'll sit exposed only in the testing
> > > branch until I rebase on rc1.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> >
> > Thank you for the patch applied.
> > I have one question about the previous already applied patchset
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > I see this patchset already merged to linux-next more than a month ago.
> > But it's still not available in the linux stable branch. Could you please
> > explain what's the problem with this one? Was some bug found during
> > linux-next testing stage? Should I fix something?
> >
> > Appreciate any help to understand what's I missing.
>
> It's not a fix so it will go in during the merge window in about 2 weeks time.
> Won't get backported to Stable though unless we ask for that to happen as it's
> not really a fix so I didn't add a marking for it to be picked up for stable
> (which would only happen after 5.20-rc1 anyway).
>
> J
Thanks a lot for explanation!
--
Thank you,
Dmitry