| Message ID | 20210219111501.14261-14-arnaud.pouliquen@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | introduce a generic IOCTL interface for RPMsg channels management | expand |
On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote: > Introduce the __rpmsg_chrdev_create_eptdev internal function that returns > the rpmsg_eptdev context structure. > This patch prepares the introduction of a RPMsg device for the > char device. the RPMsg device will need a reference to the context. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 09ae1304837c..66dcb8845d6c 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > } > EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); > > -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > - struct rpmsg_channel_info chinfo) > +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, > + struct device *parent, > + struct rpmsg_channel_info chinfo) > { > struct rpmsg_eptdev *eptdev; > struct device *dev; > @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > > eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); > if (!eptdev) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > dev = &eptdev->dev; > eptdev->rpdev = rpdev; > @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > put_device(dev); > } > > - return ret; > + return eptdev; > > free_ept_ida: > ida_simple_remove(&rpmsg_ept_ida, dev->id); > @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > put_device(dev); > kfree(eptdev); > > - return ret; > + return ERR_PTR(ret); > +} > + > +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > + struct rpmsg_channel_info chinfo) > +{ > + struct rpmsg_eptdev *eptdev; > + > + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent? > + if (IS_ERR(eptdev)) > + return PTR_ERR(eptdev); > + > + return 0; > } > EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); > > -- > 2.17.1 >
On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote: > Introduce the __rpmsg_chrdev_create_eptdev internal function that returns > the rpmsg_eptdev context structure. Add newlines between paragraphs. > This patch prepares the introduction of a RPMsg device for the > char device. the RPMsg device will need a reference to the context. s/the/The s/RPMsg/RPMSG - throughout the patchset. As a general note please be mindful of patch changelogs. I often find myself having to decipher the ideas being conveyed. I am done reviewing this set. There are things I will want to come back to but the general goals behind the patchset are being achieved. Thanks, Mathieu > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 09ae1304837c..66dcb8845d6c 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > } > EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); > > -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > - struct rpmsg_channel_info chinfo) > +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, > + struct device *parent, > + struct rpmsg_channel_info chinfo) > { > struct rpmsg_eptdev *eptdev; > struct device *dev; > @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > > eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); > if (!eptdev) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > dev = &eptdev->dev; > eptdev->rpdev = rpdev; > @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > put_device(dev); > } > > - return ret; > + return eptdev; > > free_ept_ida: > ida_simple_remove(&rpmsg_ept_ida, dev->id); > @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > put_device(dev); > kfree(eptdev); > > - return ret; > + return ERR_PTR(ret); > +} > + > +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > + struct rpmsg_channel_info chinfo) > +{ > + struct rpmsg_eptdev *eptdev; > + > + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); > + if (IS_ERR(eptdev)) > + return PTR_ERR(eptdev); > + > + return 0; > } > EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); > > -- > 2.17.1 >
On 3/4/21 7:55 PM, Mathieu Poirier wrote: > On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote: >> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns >> the rpmsg_eptdev context structure. >> This patch prepares the introduction of a RPMsg device for the >> char device. the RPMsg device will need a reference to the context. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index 09ae1304837c..66dcb8845d6c 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) >> } >> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); >> >> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, >> - struct rpmsg_channel_info chinfo) >> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, >> + struct device *parent, >> + struct rpmsg_channel_info chinfo) >> { >> struct rpmsg_eptdev *eptdev; >> struct device *dev; >> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> >> eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); >> if (!eptdev) >> - return -ENOMEM; >> + return ERR_PTR(-ENOMEM); >> >> dev = &eptdev->dev; >> eptdev->rpdev = rpdev; >> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> put_device(dev); >> } >> >> - return ret; >> + return eptdev; >> >> free_ept_ida: >> ida_simple_remove(&rpmsg_ept_ida, dev->id); >> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> put_device(dev); >> kfree(eptdev); >> >> - return ret; >> + return ERR_PTR(ret); >> +} >> + >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, >> + struct rpmsg_channel_info chinfo) >> +{ >> + struct rpmsg_eptdev *eptdev; >> + >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); > > Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent? This keep the legacy hierarchy: https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_char.c#L362 Thanks, Arnaud > >> + if (IS_ERR(eptdev)) >> + return PTR_ERR(eptdev); >> + >> + return 0; >> } >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); >> >> -- >> 2.17.1 >>
Hi Mathieu On 3/4/21 8:05 PM, Mathieu Poirier wrote: > On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote: >> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns >> the rpmsg_eptdev context structure. > > Add newlines between paragraphs. > >> This patch prepares the introduction of a RPMsg device for the >> char device. the RPMsg device will need a reference to the context. > > s/the/The > > s/RPMsg/RPMSG - throughout the patchset. > > As a general note please be mindful of patch changelogs. I often find myself > having to decipher the ideas being conveyed. Sure, i will rewrite changelogs and comments to make them more explicit. > > I am done reviewing this set. There are things I will want to come back to but > the general goals behind the patchset are being achieved. Thanks for the review! So I'm going to move forward with this approach. For the next revision I would propose, to simplify the review, to remove patches related to the RPMSG_CREATE_DEV_IOCTL. Thanks, Arnaud > > Thanks, > Mathieu > >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index 09ae1304837c..66dcb8845d6c 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) >> } >> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); >> >> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, >> - struct rpmsg_channel_info chinfo) >> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, >> + struct device *parent, >> + struct rpmsg_channel_info chinfo) >> { >> struct rpmsg_eptdev *eptdev; >> struct device *dev; >> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> >> eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); >> if (!eptdev) >> - return -ENOMEM; >> + return ERR_PTR(-ENOMEM); >> >> dev = &eptdev->dev; >> eptdev->rpdev = rpdev; >> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> put_device(dev); >> } >> >> - return ret; >> + return eptdev; >> >> free_ept_ida: >> ida_simple_remove(&rpmsg_ept_ida, dev->id); >> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> put_device(dev); >> kfree(eptdev); >> >> - return ret; >> + return ERR_PTR(ret); >> +} >> + >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, >> + struct rpmsg_channel_info chinfo) >> +{ >> + struct rpmsg_eptdev *eptdev; >> + >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); >> + if (IS_ERR(eptdev)) >> + return PTR_ERR(eptdev); >> + >> + return 0; >> } >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); >> >> -- >> 2.17.1 >>
On Fri, Mar 05, 2021 at 11:46:47AM +0100, Arnaud POULIQUEN wrote: > > > On 3/4/21 7:55 PM, Mathieu Poirier wrote: > > On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote: > >> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns > >> the rpmsg_eptdev context structure. > >> This patch prepares the introduction of a RPMsg device for the > >> char device. the RPMsg device will need a reference to the context. > >> > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > >> --- > >> drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- > >> 1 file changed, 18 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > >> index 09ae1304837c..66dcb8845d6c 100644 > >> --- a/drivers/rpmsg/rpmsg_char.c > >> +++ b/drivers/rpmsg/rpmsg_char.c > >> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > >> } > >> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); > >> > >> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > >> - struct rpmsg_channel_info chinfo) > >> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, > >> + struct device *parent, > >> + struct rpmsg_channel_info chinfo) > >> { > >> struct rpmsg_eptdev *eptdev; > >> struct device *dev; > >> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > >> > >> eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); > >> if (!eptdev) > >> - return -ENOMEM; > >> + return ERR_PTR(-ENOMEM); > >> > >> dev = &eptdev->dev; > >> eptdev->rpdev = rpdev; > >> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > >> put_device(dev); > >> } > >> > >> - return ret; > >> + return eptdev; > >> > >> free_ept_ida: > >> ida_simple_remove(&rpmsg_ept_ida, dev->id); > >> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > >> put_device(dev); > >> kfree(eptdev); > >> > >> - return ret; > >> + return ERR_PTR(ret); > >> +} > >> + > >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, > >> + struct rpmsg_channel_info chinfo) > >> +{ > >> + struct rpmsg_eptdev *eptdev; > >> + > >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); > > > > Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent? > > This keep the legacy hierarchy: > https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_char.c#L362 In patch 12 it is clear the first and second arguments are ctrldev->rpdev and ctrldev->dev. In this set the second arguments becomes rpdev->dev, which is different than ctrldev->dev. Goind back to rpmsg_ctrl_probe() we have: dev = &ctrldev->dev; device_initialize(dev); dev->parent = &rpdev->dev; As such in __rpmsg_chrdev_create_eptdev(), eptdev->dev->parent becomes ctrldev->dev->parent rather than ctrldev->dev. > > Thanks, > Arnaud > > > > > >> + if (IS_ERR(eptdev)) > >> + return PTR_ERR(eptdev); > >> + > >> + return 0; > >> } > >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); > >> > >> -- > >> 2.17.1 > >>
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 09ae1304837c..66dcb8845d6c 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) } EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy); -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, - struct rpmsg_channel_info chinfo) +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, + struct device *parent, + struct rpmsg_channel_info chinfo) { struct rpmsg_eptdev *eptdev; struct device *dev; @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL); if (!eptdev) - return -ENOMEM; + return ERR_PTR(-ENOMEM); dev = &eptdev->dev; eptdev->rpdev = rpdev; @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent put_device(dev); } - return ret; + return eptdev; free_ept_ida: ida_simple_remove(&rpmsg_ept_ida, dev->id); @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent put_device(dev); kfree(eptdev); - return ret; + return ERR_PTR(ret); +} + +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent, + struct rpmsg_channel_info chinfo) +{ + struct rpmsg_eptdev *eptdev; + + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo); + if (IS_ERR(eptdev)) + return PTR_ERR(eptdev); + + return 0; } EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
Introduce the __rpmsg_chrdev_create_eptdev internal function that returns the rpmsg_eptdev context structure. This patch prepares the introduction of a RPMsg device for the char device. the RPMsg device will need a reference to the context. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)