[PATCH] event/dsw: fix missing device pointer

[PATCH] event/dsw: fix missing device pointer

[Bruce Richardson]

After calling rte_event_dev_info_get() the ".dev" field of the info
structure should have a pointer to the underlying device, allowing the
user to e.g. get the device name using using rte_dev_name(info.dev).

The distributed software eventdev info structure did not return a
correct device pointer, though, instead returning NULL, which caused
crashes getting "rte_dev_name". Initializing the dev pointer inside the
"eventdev" struct in the device probe function fixes this by ensuring we
have a valid pointer to return in info_get calls.

Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
Cc: mattias.ronnblom@ericsson.com

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/event/dsw/dsw_evdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/event/dsw/dsw_evdev.c b/drivers/event/dsw/dsw_evdev.c
index 785c12f61f..44da3c60d1 100644
--- a/drivers/event/dsw/dsw_evdev.c
+++ b/drivers/event/dsw/dsw_evdev.c
@@ -440,6 +440,7 @@ dsw_probe(struct rte_vdev_device *vdev)
 		return -EFAULT;
 
 	dev->dev_ops = &dsw_evdev_ops;
+	dev->dev = &vdev->device;
 	dev->enqueue = dsw_event_enqueue;
 	dev->enqueue_burst = dsw_event_enqueue_burst;
 	dev->enqueue_new_burst = dsw_event_enqueue_new_burst;
-- 
2.39.2

Re: [PATCH] event/dsw: fix missing device pointer

[Bruce Richardson]

On Tue, Oct 17, 2023 at 04:45:32PM +0100, Bruce Richardson wrote:
> After calling rte_event_dev_info_get() the ".dev" field of the info
> structure should have a pointer to the underlying device, allowing the
> user to e.g. get the device name using using rte_dev_name(info.dev).
> 
> The distributed software eventdev info structure did not return a correct
> device pointer, though, instead returning NULL, which caused crashes
> getting "rte_dev_name". Initializing the dev pointer inside the
> "eventdev" struct in the device probe function fixes this by ensuring we
> have a valid pointer to return in info_get calls.
> 
> Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build
> system") Cc: mattias.ronnblom@ericsson.com
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> ---
Since checkpatch has flagged a duplicated word in my commit log above,
copy-pasted from the equivalent event/sw patch fix, I'm sending a v2 of the
two together in a patchset. See thread [1] for that v2.

/Bruce

[1] https://inbox.dpdk.org/dev/20231017155148.153095-1-bruce.richardson@intel.com/

Re: [PATCH] event/dsw: fix missing device pointer

[Jerin Jacob]

On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> After calling rte_event_dev_info_get() the ".dev" field of the info
> structure should have a pointer to the underlying device, allowing the
> user to e.g. get the device name using using rte_dev_name(info.dev).
>
> The distributed software eventdev info structure did not return a
> correct device pointer, though, instead returning NULL, which caused
> crashes getting "rte_dev_name". Initializing the dev pointer inside the
> "eventdev" struct in the device probe function fixes this by ensuring we
> have a valid pointer to return in info_get calls.
>
> Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> Cc: mattias.ronnblom@ericsson.com
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Is this issue for all "vdev" devices? if so, Please check for
drivers/event/skeleton too.

> ---
>  drivers/event/dsw/dsw_evdev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/event/dsw/dsw_evdev.c b/drivers/event/dsw/dsw_evdev.c
> index 785c12f61f..44da3c60d1 100644
> --- a/drivers/event/dsw/dsw_evdev.c
> +++ b/drivers/event/dsw/dsw_evdev.c
> @@ -440,6 +440,7 @@ dsw_probe(struct rte_vdev_device *vdev)
>                 return -EFAULT;
>
>         dev->dev_ops = &dsw_evdev_ops;
> +       dev->dev = &vdev->device;
>         dev->enqueue = dsw_event_enqueue;
>         dev->enqueue_burst = dsw_event_enqueue_burst;
>         dev->enqueue_new_burst = dsw_event_enqueue_new_burst;
> --
> 2.39.2
>

Re: [PATCH] event/dsw: fix missing device pointer

[David Marchand]

On Tue, Oct 17, 2023 at 6:04 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > After calling rte_event_dev_info_get() the ".dev" field of the info
> > structure should have a pointer to the underlying device, allowing the
> > user to e.g. get the device name using using rte_dev_name(info.dev).
> >
> > The distributed software eventdev info structure did not return a
> > correct device pointer, though, instead returning NULL, which caused
> > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > "eventdev" struct in the device probe function fixes this by ensuring we
> > have a valid pointer to return in info_get calls.
> >
> > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > Cc: mattias.ronnblom@ericsson.com
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Is this issue for all "vdev" devices? if so, Please check for
> drivers/event/skeleton too.

Should we add some eventdev wrappers for shared code like this?
Something like rte_eth_dev_pci_generic_probe() / rte_eth_vdev_allocate().


-- 
David Marchand

Re: [PATCH] event/dsw: fix missing device pointer

[Bruce Richardson]

On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
> On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > After calling rte_event_dev_info_get() the ".dev" field of the info
> > structure should have a pointer to the underlying device, allowing the
> > user to e.g. get the device name using using rte_dev_name(info.dev).
> >
> > The distributed software eventdev info structure did not return a
> > correct device pointer, though, instead returning NULL, which caused
> > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > "eventdev" struct in the device probe function fixes this by ensuring we
> > have a valid pointer to return in info_get calls.
> >
> > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > Cc: mattias.ronnblom@ericsson.com
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> Is this issue for all "vdev" devices? if so, Please check for
> drivers/event/skeleton too.
> 
Yes, good point, looks like event/skeleton also returns NULL for the device
pointer.

I'll do up a v3 with the extra patch in it.

/Bruce

Re: [PATCH] event/dsw: fix missing device pointer

[Jerin Jacob]

On Tue, Oct 17, 2023 at 9:45 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
> > On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > After calling rte_event_dev_info_get() the ".dev" field of the info
> > > structure should have a pointer to the underlying device, allowing the
> > > user to e.g. get the device name using using rte_dev_name(info.dev).
> > >
> > > The distributed software eventdev info structure did not return a
> > > correct device pointer, though, instead returning NULL, which caused
> > > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > > "eventdev" struct in the device probe function fixes this by ensuring we
> > > have a valid pointer to return in info_get calls.
> > >
> > > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > > Cc: mattias.ronnblom@ericsson.com
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > Is this issue for all "vdev" devices? if so, Please check for
> > drivers/event/skeleton too.
> >
> Yes, good point, looks like event/skeleton also returns NULL for the device
> pointer.
>
> I'll do up a v3 with the extra patch in it.

Looks there are more vdev devuces. Can we have common PMD function or
extend rte_event_pmd_vdev_init or so.

[main]dell[dpdk.org] $ git grep rte_event_pmd_vdev_init
drivers/event/dpaa/dpaa_eventdev.c:     eventdev = rte_event_pmd_vdev_init(name,
drivers/event/dpaa2/dpaa2_eventdev.c:   eventdev = rte_event_pmd_vdev_init(name,
drivers/event/dsw/dsw_evdev.c:  dev = rte_event_pmd_vdev_init(name,
sizeof(struct dsw_evdev),
drivers/event/octeontx/ssovf_evdev.c:   eventdev =
rte_event_pmd_vdev_init(name, sizeof(struct ssovf_evdev),
drivers/event/opdl/opdl_evdev.c:        dev = rte_event_pmd_vdev_init(name,
drivers/event/skeleton/skeleton_eventdev.c:     eventdev =
rte_event_pmd_vdev_init(name,
drivers/event/sw/sw_evdev.c:    dev = rte_event_pmd_vdev_init(name,
lib/eventdev/eventdev_pmd_vdev.h:rte_event_pmd_vdev_init(const char
*name, size_t dev_private_size,
lib/eventdev/version.map:       rte_event_pmd_vdev_init;


> /Bruce

Re: [PATCH] event/dsw: fix missing device pointer

[Jerin Jacob]

On Tue, Oct 17, 2023 at 10:21 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 9:45 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
> > > On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > After calling rte_event_dev_info_get() the ".dev" field of the info
> > > > structure should have a pointer to the underlying device, allowing the
> > > > user to e.g. get the device name using using rte_dev_name(info.dev).
> > > >
> > > > The distributed software eventdev info structure did not return a
> > > > correct device pointer, though, instead returning NULL, which caused
> > > > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > > > "eventdev" struct in the device probe function fixes this by ensuring we
> > > > have a valid pointer to return in info_get calls.
> > > >
> > > > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > > > Cc: mattias.ronnblom@ericsson.com
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > Is this issue for all "vdev" devices? if so, Please check for
> > > drivers/event/skeleton too.
> > >
> > Yes, good point, looks like event/skeleton also returns NULL for the device
> > pointer.
> >
> > I'll do up a v3 with the extra patch in it.
>
> Looks there are more vdev devuces. Can we have common PMD function or
> extend rte_event_pmd_vdev_init or so.


@Richardson, Bruce I will be on vacation from Friday, So would like to
give PR for rc2 before that.

Adding helper function in rc2 may be risky, Could you fix all vdev
mentioned below.
Helper work, I think, we can take in next release.


>
> [main]dell[dpdk.org] $ git grep rte_event_pmd_vdev_init
> drivers/event/dpaa/dpaa_eventdev.c:     eventdev = rte_event_pmd_vdev_init(name,
> drivers/event/dpaa2/dpaa2_eventdev.c:   eventdev = rte_event_pmd_vdev_init(name,
> drivers/event/dsw/dsw_evdev.c:  dev = rte_event_pmd_vdev_init(name,
> sizeof(struct dsw_evdev),
> drivers/event/octeontx/ssovf_evdev.c:   eventdev =
> rte_event_pmd_vdev_init(name, sizeof(struct ssovf_evdev),
> drivers/event/opdl/opdl_evdev.c:        dev = rte_event_pmd_vdev_init(name,
> drivers/event/skeleton/skeleton_eventdev.c:     eventdev =
> rte_event_pmd_vdev_init(name,
> drivers/event/sw/sw_evdev.c:    dev = rte_event_pmd_vdev_init(name,
> lib/eventdev/eventdev_pmd_vdev.h:rte_event_pmd_vdev_init(const char
> *name, size_t dev_private_size,
> lib/eventdev/version.map:       rte_event_pmd_vdev_init;
>
>
> > /Bruce

Re: [PATCH] event/dsw: fix missing device pointer

[Mattias Rönnblom]

On 2023-10-18 07:18, Jerin Jacob wrote:
> On Tue, Oct 17, 2023 at 10:21 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Tue, Oct 17, 2023 at 9:45 PM Bruce Richardson
>> <bruce.richardson@intel.com> wrote:
>>>
>>> On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
>>>> On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
>>>> <bruce.richardson@intel.com> wrote:
>>>>>
>>>>> After calling rte_event_dev_info_get() the ".dev" field of the info
>>>>> structure should have a pointer to the underlying device, allowing the
>>>>> user to e.g. get the device name using using rte_dev_name(info.dev).
>>>>>
>>>>> The distributed software eventdev info structure did not return a
>>>>> correct device pointer, though, instead returning NULL, which caused
>>>>> crashes getting "rte_dev_name". Initializing the dev pointer inside the
>>>>> "eventdev" struct in the device probe function fixes this by ensuring we
>>>>> have a valid pointer to return in info_get calls.
>>>>>
>>>>> Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
>>>>> Cc: mattias.ronnblom@ericsson.com
>>>>>
>>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>>
>>>> Is this issue for all "vdev" devices? if so, Please check for
>>>> drivers/event/skeleton too.
>>>>
>>> Yes, good point, looks like event/skeleton also returns NULL for the device
>>> pointer.
>>>
>>> I'll do up a v3 with the extra patch in it.
>>
>> Looks there are more vdev devuces. Can we have common PMD function or
>> extend rte_event_pmd_vdev_init or so.
> 
> 
> @Richardson, Bruce I will be on vacation from Friday, So would like to
> give PR for rc2 before that.
> 
> Adding helper function in rc2 may be risky, Could you fix all vdev
> mentioned below.
> Helper work, I think, we can take in next release.
> 

I agree.

Thanks Bruce!

> 
>>
>> [main]dell[dpdk.org] $ git grep rte_event_pmd_vdev_init
>> drivers/event/dpaa/dpaa_eventdev.c:     eventdev = rte_event_pmd_vdev_init(name,
>> drivers/event/dpaa2/dpaa2_eventdev.c:   eventdev = rte_event_pmd_vdev_init(name,
>> drivers/event/dsw/dsw_evdev.c:  dev = rte_event_pmd_vdev_init(name,
>> sizeof(struct dsw_evdev),
>> drivers/event/octeontx/ssovf_evdev.c:   eventdev =
>> rte_event_pmd_vdev_init(name, sizeof(struct ssovf_evdev),
>> drivers/event/opdl/opdl_evdev.c:        dev = rte_event_pmd_vdev_init(name,
>> drivers/event/skeleton/skeleton_eventdev.c:     eventdev =
>> rte_event_pmd_vdev_init(name,
>> drivers/event/sw/sw_evdev.c:    dev = rte_event_pmd_vdev_init(name,
>> lib/eventdev/eventdev_pmd_vdev.h:rte_event_pmd_vdev_init(const char
>> *name, size_t dev_private_size,
>> lib/eventdev/version.map:       rte_event_pmd_vdev_init;
>>
>>
>>> /Bruce

Re: [PATCH] event/dsw: fix missing device pointer

[Bruce Richardson]

On Wed, Oct 18, 2023 at 10:48:14AM +0530, Jerin Jacob wrote:
> On Tue, Oct 17, 2023 at 10:21 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 9:45 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
> > > > On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > After calling rte_event_dev_info_get() the ".dev" field of the info
> > > > > structure should have a pointer to the underlying device, allowing the
> > > > > user to e.g. get the device name using using rte_dev_name(info.dev).
> > > > >
> > > > > The distributed software eventdev info structure did not return a
> > > > > correct device pointer, though, instead returning NULL, which caused
> > > > > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > > > > "eventdev" struct in the device probe function fixes this by ensuring we
> > > > > have a valid pointer to return in info_get calls.
> > > > >
> > > > > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > > > > Cc: mattias.ronnblom@ericsson.com
> > > > >
> > > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >
> > > > Is this issue for all "vdev" devices? if so, Please check for
> > > > drivers/event/skeleton too.
> > > >
> > > Yes, good point, looks like event/skeleton also returns NULL for the device
> > > pointer.
> > >
> > > I'll do up a v3 with the extra patch in it.
> >
> > Looks there are more vdev devuces. Can we have common PMD function or
> > extend rte_event_pmd_vdev_init or so.
> 
> 
> @Richardson, Bruce I will be on vacation from Friday, So would like to
> give PR for rc2 before that.
> 
> Adding helper function in rc2 may be risky, Could you fix all vdev
> mentioned below.
> Helper work, I think, we can take in next release.
> 
Yes, I was going to reply with some similar sentiment. I think it would be
risky to try and do a proper solution in a hurry. I will attempt to fix all
vdevs for rc2.

/Bruce

Re: [PATCH] event/dsw: fix missing device pointer

[Bruce Richardson]

On Wed, Oct 18, 2023 at 10:48:14AM +0530, Jerin Jacob wrote:
> On Tue, Oct 17, 2023 at 10:21 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 9:45 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 09:34:04PM +0530, Jerin Jacob wrote:
> > > > On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > After calling rte_event_dev_info_get() the ".dev" field of the info
> > > > > structure should have a pointer to the underlying device, allowing the
> > > > > user to e.g. get the device name using using rte_dev_name(info.dev).
> > > > >
> > > > > The distributed software eventdev info structure did not return a
> > > > > correct device pointer, though, instead returning NULL, which caused
> > > > > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > > > > "eventdev" struct in the device probe function fixes this by ensuring we
> > > > > have a valid pointer to return in info_get calls.
> > > > >
> > > > > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > > > > Cc: mattias.ronnblom@ericsson.com
> > > > >
> > > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >
> > > > Is this issue for all "vdev" devices? if so, Please check for
> > > > drivers/event/skeleton too.
> > > >
> > > Yes, good point, looks like event/skeleton also returns NULL for the device
> > > pointer.
> > >
> > > I'll do up a v3 with the extra patch in it.
> >
> > Looks there are more vdev devuces. Can we have common PMD function or
> > extend rte_event_pmd_vdev_init or so.
> 
> 
> @Richardson, Bruce I will be on vacation from Friday, So would like to
> give PR for rc2 before that.
> 
> Adding helper function in rc2 may be risky, Could you fix all vdev
> mentioned below.
> Helper work, I think, we can take in next release.
> 

Having looked at it more, and considering I cannot test a number of the
drivers (dpaa*, octeon), I actually think the safest approach is to modify
the vdev_init function. It's a small change, so I think it's pretty low
risk. Patch will follow shortly.

/Bruce