Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic
On Wed, Sep 30, 2020 at 09:13:27AM +0200, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:10:00PM -0600, Mathieu Poirier wrote:
> > Make name service module transport agnostic by using the rpmsg
> > device specific byte conversion routine.
> >
> > Signed-off-by: Mathieu Poirier
> > ---
> > drivers/rpmsg/rpmsg_ns.c | 10 --
> > include/linux/rpmsg_ns.h | 4 ++--
> > 2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> > index b3318bf84433..1df3aaadfe10 100644
> > --- a/drivers/rpmsg/rpmsg_ns.c
> > +++ b/drivers/rpmsg/rpmsg_ns.c
> > @@ -18,9 +18,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> > *data, int len,
> > struct rpmsg_ns_msg *msg = data;
> > struct rpmsg_device *newch;
> > struct rpmsg_channel_info chinfo;
> > - struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> > - struct virtproc_info *vrp = vch->vrp;
> > - struct device *dev = >vdev->dev;
> > + struct device *dev = >dev;
> > int ret;
> >
> > #if defined(CONFIG_DYNAMIC_DEBUG)
> > @@ -38,13 +36,13 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> > *data, int len,
> >
> > strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> > chinfo.src = RPMSG_ADDR_ANY;
> > - chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
> > + chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> >
> > dev_info(dev, "%sing channel %s addr 0x%x\n",
> > -virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> > +rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> > "destroy" : "creat", msg->name, chinfo.dst);
> >
> > - if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> > + if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) {
> > ret = rpmsg_release_channel(rpdev, );
> > if (ret)
> > dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> > diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> > index aabc6c3c0d6d..9f3030b2145b 100644
> > --- a/include/linux/rpmsg_ns.h
> > +++ b/include/linux/rpmsg_ns.h
> > @@ -41,8 +41,8 @@ struct rpmsg_hdr {
> > */
> > struct rpmsg_ns_msg {
> > char name[RPMSG_NAME_SIZE];
> > - __virtio32 addr;
> > - __virtio32 flags;
> > + u32 addr;
> > + u32 flags;
>
> These aren't (necessarily) native endianness, right? Don't they deserve a
> separate type? __rpmsg32?
>
I kept them as u32 type to be as generic as possible and let the endianness
conversion callback do the work. Making them a __rpmsg32 is a good idea, it
would force a proper implementation.
> Thanks
> Guennadi
>
> > } __packed;
> >
> > /**
> > --
> > 2.25.1
> >
Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic
On Mon, Sep 21, 2020 at 06:10:00PM -0600, Mathieu Poirier wrote:
> Make name service module transport agnostic by using the rpmsg
> device specific byte conversion routine.
>
> Signed-off-by: Mathieu Poirier
> ---
> drivers/rpmsg/rpmsg_ns.c | 10 --
> include/linux/rpmsg_ns.h | 4 ++--
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index b3318bf84433..1df3aaadfe10 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -18,9 +18,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> struct rpmsg_ns_msg *msg = data;
> struct rpmsg_device *newch;
> struct rpmsg_channel_info chinfo;
> - struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> - struct virtproc_info *vrp = vch->vrp;
> - struct device *dev = >vdev->dev;
> + struct device *dev = >dev;
> int ret;
>
> #if defined(CONFIG_DYNAMIC_DEBUG)
> @@ -38,13 +36,13 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
>
> strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> chinfo.src = RPMSG_ADDR_ANY;
> - chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
> + chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
>
> dev_info(dev, "%sing channel %s addr 0x%x\n",
> - virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> + rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
>"destroy" : "creat", msg->name, chinfo.dst);
>
> - if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> + if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) {
> ret = rpmsg_release_channel(rpdev, );
> if (ret)
> dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> index aabc6c3c0d6d..9f3030b2145b 100644
> --- a/include/linux/rpmsg_ns.h
> +++ b/include/linux/rpmsg_ns.h
> @@ -41,8 +41,8 @@ struct rpmsg_hdr {
> */
> struct rpmsg_ns_msg {
> char name[RPMSG_NAME_SIZE];
> - __virtio32 addr;
> - __virtio32 flags;
> + u32 addr;
> + u32 flags;
These aren't (necessarily) native endianness, right? Don't they deserve a
separate type? __rpmsg32?
Thanks
Guennadi
> } __packed;
>
> /**
> --
> 2.25.1
>
Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic
Hi Mathieu,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20200921]
[cannot apply to linux/master linus/master rpmsg/for-next v5.9-rc6 v5.9-rc5
v5.9-rc4 v5.9-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
base:b10b8ad862118bf42c28a98b0f067619aadcfb23
config: i386-randconfig-s001-20200921 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-201-g24bdaac6-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
sparse warnings: (new ones prefixed by >>)
drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse: sparse: incorrect type in
argument 2 (different base types) @@ expected restricted __virtio16
[usertype] val @@ got unsigned short [usertype] val @@
drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse: expected restricted
__virtio16 [usertype] val
drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse: got unsigned short
[usertype] val
drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse: sparse: incorrect type in
return expression (different base types) @@ expected unsigned short @@
got restricted __virtio16 @@
drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse: expected unsigned short
drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse: got restricted
__virtio16
drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse: sparse: incorrect type in
argument 2 (different base types) @@ expected restricted __virtio32
[usertype] val @@ got unsigned int [usertype] val @@
drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse: expected restricted
__virtio32 [usertype] val
drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse: got unsigned int
[usertype] val
drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse: sparse: incorrect type in
return expression (different base types) @@ expected unsigned int @@
got restricted __virtio32 @@
drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse: expected unsigned int
drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse: got restricted
__virtio32
>> drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse: sparse: incorrect type in
>> assignment (different base types) @@ expected unsigned int [addressable]
>> [usertype] addr @@ got restricted __virtio32 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse: expected unsigned int
>> [addressable] [usertype] addr
drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse: got restricted
__virtio32
>> drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse: sparse: incorrect type in
>> assignment (different base types) @@ expected unsigned int [addressable]
>> [usertype] flags @@ got restricted __virtio32 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse: expected unsigned int
>> [addressable] [usertype] flags
drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse: got restricted
__virtio32
drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse: sparse: incorrect type in
assignment (different base types) @@ expected unsigned int [addressable]
[usertype] addr @@ got restricted __virtio32 @@
drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse: expected unsigned int
[addressable] [usertype] addr
drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse: got restricted
__virtio32
drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse: sparse: incorrect type in
assignment (different base types) @@ expected unsigned int [addressable]
[usertype] flags @@ got restricted __virtio32 @@
drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse: expected unsigned int
[addressable] [usertype] flags
drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse: got restricted
__virtio32
#
https://github.com/0day-ci/linux/commit/ab159ea48198df2ab06ff9fe97e63cca354bff20
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
git checkout ab159ea48198df2ab06ff9fe97e63cca354bff20
vim +267 drivers/rpmsg/virtio_rpmsg_bus.c
dd032e0b67fcd61 Mathieu Poirier 2020-09-21 167
dd032e0b67fcd61 Mathieu Poirier 2020-09-21 168 static u16
virtio_rpmsg_cpu_to_transport16(struct rpmsg_device *rpdev, u16 val)
dd032e0b67fcd61 Mathieu Poirier 2020-09-21 169 {
dd032e0b67fcd61 Mathieu Poirier 2020-09-21 170 struct
virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
dd032e0b67fcd61 Mathieu Poirier 2020-09-21 171 struct virtproc_info
*vrp = vch->vrp;
dd032e0b67fcd61 Mathieu
