HI Andy,
> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Wednesday, January 29, 2025 12:24 PM
> To: Mohan, Subramanian <subramanian.mohan@xxxxxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; akpm@linux-
> foundation.org; greg@xxxxxxxxx; corbet@xxxxxxx; Hall, Christopher S
> <christopher.s.hall@xxxxxxxxx>; tglx@xxxxxxxxxxxxx; Dong, Eddie
> <eddie.dong@xxxxxxxxx>; N, Pandith <pandith.n@xxxxxxxxx>; T R, Thejesh
> Reddy <thejesh.reddy.t.r@xxxxxxxxx>; Zage, David <david.zage@xxxxxxxxx>;
> Chinnadurai, Srinivasan <srinivasan.chinnadurai@xxxxxxxxx>;
> rdunlap@xxxxxxxxxxxxx; bagasdotme@xxxxxxxxx; giometti@xxxxxxxxxxxx
> 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@xxxxxxxxx
> wrote:
> > From: Subramanian Mohan <subramanian.mohan@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
>
> ...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
>
