[PATCH v2 1/1] pps: retrieve generator specific data from framework

[PATCH v2 1/1] pps: retrieve generator specific data from framework

[subramanian.mohan]

From: Subramanian Mohan <subramanian.mohan@intel.com>

While adapting pps generator driver(tio generator as an example)to the new
generator framework, As part of driver registration the pps_gen_device
pointer is returned from framework. Due to which there is difficulty in
getting generator driver data back in enable function. we won’t be able
to use container_of macro as it results in static assert. we might end up
in using static pointer. To avoid the same and get back the generator
driver data back we are proposing generic approach to add drv_prv_data
pointer inside the struct pps_gen_source_info.

Example TIO structure wrapped with pps_gen_device and usage.

struct pps_tio {
	/* Framework Related * /
	struct pps_gen_source_info pps_tio_source_info
	struct pps_gen_device *pps_gen;

	/* TIO Specific Data */
};

static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable) {

    /* Getting TIO data back */
    /* Note: drv_prv_data will be initialized in our init routine */
    struct pps_tio *tio = pps_gen->info.drv_prv_data;

    /* Access tio members here to set some of the parameters */

    return 0;
}

V1 -> V2:
    * Updated reviewers.

Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
---
 include/linux/pps_gen_kernel.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
index 022ea0ac4440..8484bb2828ef 100644
--- a/include/linux/pps_gen_kernel.h
+++ b/include/linux/pps_gen_kernel.h
@@ -35,6 +35,7 @@ struct pps_gen_source_info {
 	int (*get_time)(struct pps_gen_device *pps_gen,
 					struct timespec64 *time);
 	int (*enable)(struct pps_gen_device *pps_gen, bool enable);
+	void *drv_prv_data;
 
 /* private: internal use only */
 	struct module *owner;
-- 
2.35.3

Re: [PATCH v2 1/1] pps: retrieve generator specific data from framework

[Andy Shevchenko]

On Tue, Jan 28, 2025 at 07:47:43PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> While adapting pps generator driver(tio generator as an example)to the new
> generator framework, As part of driver registration the pps_gen_device
> pointer is returned from framework. Due to which there is difficulty in
> getting generator driver data back in enable function. we won’t be able
> to use container_of macro as it results in static assert. we might end up

container_of()

Can you be more specific, what exactly happens?

> in using static pointer. To avoid the same and get back the generator
> driver data back we are proposing generic approach to add drv_prv_data
> pointer inside the struct pps_gen_source_info.

> Example TIO structure wrapped with pps_gen_device and usage.
> 
> struct pps_tio {
> 	/* Framework Related * /
> 	struct pps_gen_source_info pps_tio_source_info
> 	struct pps_gen_device *pps_gen;

> };

> static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable) {
> 
>     /* Getting TIO data back */
>     /* Note: drv_prv_data will be initialized in our init routine */
>     struct pps_tio *tio = pps_gen->info.drv_prv_data;

So, what's wrong with the container_of() against pps_gen->info?
We have tons of code in the kernel that does it.

>     /* Access tio members here to set some of the parameters */
> 
>     return 0;
> }

Okay, looking into the implementation I see what's the issue (but it doesn't
mean that commit message is good here, you still need to explain better why
you can't get the correct address).

The problem is that info is got fully copied. Perhaps we should rely on
the fact that it always be provided? I dunno any data structure that defines
callbacks and other "info" material should be considered as temporary storage
(on stack), it makes not much sense.

...

> V1 -> V2:
>     * Updated reviewers.

Please, use...

> Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
> ---

...this place for the comments and changelogs.

...

> +	void *drv_prv_data;

If really needed we should probably use a better name, like "private".

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 1/1] pps: retrieve generator specific data from framework

[Mohan, Subramanian]

HI Andy, 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, January 29, 2025 12:24 PM
> To: Mohan, Subramanian <subramanian.mohan@intel.com>
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; akpm@linux-
> foundation.org; greg@kroah.com; corbet@lwn.net; Hall, Christopher S
> <christopher.s.hall@intel.com>; tglx@linutronix.de; Dong, Eddie
> <eddie.dong@intel.com>; N, Pandith <pandith.n@intel.com>; T R, Thejesh
> Reddy <thejesh.reddy.t.r@intel.com>; Zage, David <david.zage@intel.com>;
> Chinnadurai, Srinivasan <srinivasan.chinnadurai@intel.com>;
> rdunlap@infradead.org; bagasdotme@gmail.com; giometti@enneenne.com
> Subject: Re: [PATCH v2 1/1] pps: retrieve generator specific data from
> framework
> 
> On Tue, Jan 28, 2025 at 07:47:43PM +0530, subramanian.mohan@intel.com
> wrote:
> > From: Subramanian Mohan <subramanian.mohan@intel.com>
> >
> > While adapting pps generator driver(tio generator as an example)to the
> > new generator framework, As part of driver registration the
> > pps_gen_device pointer is returned from framework. Due to which there
> > is difficulty in getting generator driver data back in enable
> > function. we won't be able to use container_of macro as it results in
> > static assert. we might end up
> 
> container_of()
> 
> Can you be more specific, what exactly happens?
Let me provide some more info:

/* PPS Gen  Device  main struct*/
struct pps_gen_device {
	struct pps_gen_source_info info;	/* PSS generator info */
	bool enabled;				/* PSS generator status */
	......
	/* More members */
}

/* TIO struct */

struct pps_tio {
	struct pps_gen_source_info pps_tio_source_info;
	struct pps_gen_device *pps_gen;
	struct hrtimer timer;
	struct device *dev;
	spinlock_t lock;
	........
	/* More members */
};

Once the framework registration is done in probe function through pps_gen_register_source we get back 
pps_gen pointer and stored in our struct pps_tio.

As part of enable function:
We are trying to get back pps_tio (parent structure) with the help of pps_gen  as this is pointer container_of does not work here.
Hence proposed add a new  generic  private pointer inside the  pps_gen_source_info structure which is exposed to driver.

static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable)
{
    /* Trying to get back pps_tio struct is not possible as pps_gen is a  pointer in structure */
     container_of(pps_gen, struct pps_tio, pps_gen);

}

Compilation Error:
In file included from ./include/linux/bitfield.h:10,
                 from drivers/pps/generators/pps_gen_tio.c:8:
drivers/pps/generators/pps_gen_tio.c: In function 'pps_tio_gen_enable':
./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
./include/linux/container_of.h:20:2: note: in expansion of macro 'static_assert'
   20 |  static_assert(__same_type(*(ptr), ((type *)0)->member) || \
      |  ^~~~~~~~~~~~~
drivers/pps/generators/pps_gen_tio.c:172:24: note: in expansion of macro 'container_of'
  172 |  struct pps_tio *tio = container_of(pps_gen,struct pps_tio, pps_gen);

> 
> > in using static pointer. To avoid the same and get back the generator
> > driver data back we are proposing generic approach to add drv_prv_data
> > pointer inside the struct pps_gen_source_info.
> 
> > Example TIO structure wrapped with pps_gen_device and usage.
> >
> > struct pps_tio {
> > 	/* Framework Related * /
> > 	struct pps_gen_source_info pps_tio_source_info
> > 	struct pps_gen_device *pps_gen;
> 
> > };
> 
> > static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable)
> > {
> >
> >     /* Getting TIO data back */
> >     /* Note: drv_prv_data will be initialized in our init routine */
> >     struct pps_tio *tio = pps_gen->info.drv_prv_data;
> 
> So, what's wrong with the container_of() against pps_gen->info?
> We have tons of code in the kernel that does it.

We are trying to get our TIO/struct pps_tio structure back using pps_gen pointer..

> 
> >     /* Access tio members here to set some of the parameters */
> >
> >     return 0;
> > }
> 
> Okay, looking into the implementation I see what's the issue (but it doesn't
> mean that commit message is good here, you still need to explain better why
> you can't get the correct address).

Ok, Something like this:

"Adding new generic private pointer to the struct pps_gen_source_info which can help in saving the parent driver data.
 In the existing adaption using container_of with a pointer to fetch the parent driver data/address is not possible"

"Example as shown above can be given as reference"

> 
> The problem is that info is got fully copied. Perhaps we should rely on the fact
> that it always be provided? I dunno any data structure that defines callbacks
> and other "info" material should be considered as temporary storage (on
> stack), it makes not much sense.

We are trying to get our TIO/struct pps_tio structure back using pps_gen pointer..

> 
> ...
> 
> > V1 -> V2:
> >     * Updated reviewers.
> 
> Please, use...
> 
> > Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
> > ---
> 
> ...this place for the comments and changelogs.

Ok Will take care in next version.

> 
> ...
> 
> > +	void *drv_prv_data;
> 
> If really needed we should probably use a better name, like "private".

Ok, will change the member name to private.

Thanks,
Subbu

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2 1/1] pps: retrieve generator specific data from framework

[Andy Shevchenko]

On Wed, Jan 29, 2025 at 09:30:10AM +0000, Mohan, Subramanian wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Wednesday, January 29, 2025 12:24 PM
> > On Tue, Jan 28, 2025 at 07:47:43PM +0530, subramanian.mohan@intel.com
> > wrote:

...

> > > While adapting pps generator driver(tio generator as an example)to the
> > > new generator framework, As part of driver registration the
> > > pps_gen_device pointer is returned from framework. Due to which there
> > > is difficulty in getting generator driver data back in enable
> > > function. we won't be able to use container_of macro as it results in
> > > static assert. we might end up
> > 
> > container_of()
> > 
> > Can you be more specific, what exactly happens?
> Let me provide some more info:

...

> Once the framework registration is done in probe function through
> pps_gen_register_source we get back pps_gen pointer and stored in our struct
> pps_tio.

This is irrelevant.

> As part of enable function:
> We are trying to get back pps_tio (parent structure) with the help of pps_gen
> as this is pointer container_of does not work here.
> Hence proposed add a new  generic  private pointer inside the
> pps_gen_source_info structure which is exposed to driver.
> 
> static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable)
> {
>     /* Trying to get back pps_tio struct is not possible as pps_gen is a  pointer in structure */
>      container_of(pps_gen, struct pps_tio, pps_gen);
> 
> }
> 
> Compilation Error:
> In file included from ./include/linux/bitfield.h:10,
>                  from drivers/pps/generators/pps_gen_tio.c:8:
> drivers/pps/generators/pps_gen_tio.c: In function 'pps_tio_gen_enable':
> ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()"
>    78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
>       |                                         ^~~~~~~~~~~~~~
> ./include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
>    77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
>       |                                  ^~~~~~~~~~~~~~~
> ./include/linux/container_of.h:20:2: note: in expansion of macro 'static_assert'
>    20 |  static_assert(__same_type(*(ptr), ((type *)0)->member) || \
>       |  ^~~~~~~~~~~~~
> drivers/pps/generators/pps_gen_tio.c:172:24: note: in expansion of macro 'container_of'
>   172 |  struct pps_tio *tio = container_of(pps_gen,struct pps_tio, pps_gen);

Of course, you shouldn't use pointers, only embedded in members can be used
for container_of(). It seems like basic misunderstanding of how container_of()
works.

> > > in using static pointer. To avoid the same and get back the generator
> > > driver data back we are proposing generic approach to add drv_prv_data
> > > pointer inside the struct pps_gen_source_info.
> > 
> > > Example TIO structure wrapped with pps_gen_device and usage.
> > >
> > > struct pps_tio {
> > > 	/* Framework Related * /
> > > 	struct pps_gen_source_info pps_tio_source_info
> > > 	struct pps_gen_device *pps_gen;
> > 
> > > };
> > 
> > > static int pps_tio_enable(struct pps_gen_device *pps_gen, bool enable)
> > > {
> > >
> > >     /* Getting TIO data back */
> > >     /* Note: drv_prv_data will be initialized in our init routine */
> > >     struct pps_tio *tio = pps_gen->info.drv_prv_data;
> > 
> > So, what's wrong with the container_of() against pps_gen->info?
> > We have tons of code in the kernel that does it.
> 
> We are trying to get our TIO/struct pps_tio structure back using pps_gen pointer..
> 
> > >     /* Access tio members here to set some of the parameters */
> > >
> > >     return 0;
> > > }
> > 
> > Okay, looking into the implementation I see what's the issue (but it doesn't
> > mean that commit message is good here, you still need to explain better why
> > you can't get the correct address).
> 
> Ok, Something like this:
> 
> "Adding new generic private pointer to the struct pps_gen_source_info which
> can help in saving the parent driver data. In the existing adaption using
> container_of with a pointer to fetch the parent driver data/address is not

Pay attention for other comments which I gave against commit message text.

> possible"
> 
> "Example as shown above can be given as reference"
> 
> > The problem is that info is got fully copied. Perhaps we should rely on the fact
> > that it always be provided? I dunno any data structure that defines callbacks
> > and other "info" material should be considered as temporary storage (on
> > stack), it makes not much sense.
> 
> We are trying to get our TIO/struct pps_tio structure back using pps_gen
> pointer..

And..? What I proposed to replace copy of info structure in the struct
pps_gen_device by a pointer and make it const. In such a case the users
must provide either this from a global constant variable or a heap
(if for some most unlike reason they do it dynamically). That will solve
your issue and also makes memory foot print better. I.o.w. we have time
to modify this in easy way while the framework has not be spread over
the kernel.

-- 
With Best Regards,
Andy Shevchenko