diff mbox series

event/dsw: fix missing device pointer

Message ID 20231017154532.152741-1-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series event/dsw: fix missing device pointer |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch warning coding style issues
ci/loongarch-unit-testing success Unit Testing PASS

Commit Message

Bruce Richardson Oct. 17, 2023, 3:45 p.m. UTC 
  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(+)

Comments

Bruce Richardson Oct. 17, 2023, 3:55 p.m. UTC | #1 
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/
Jerin Jacob Oct. 17, 2023, 4:04 p.m. UTC | #2 
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
>
David Marchand Oct. 17, 2023, 4:11 p.m. UTC | #3 
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().
Bruce Richardson Oct. 17, 2023, 4:15 p.m. UTC | #4 
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
Jerin Jacob Oct. 17, 2023, 4:51 p.m. UTC | #5 
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
Jerin Jacob Oct. 18, 2023, 5:18 a.m. UTC | #6 
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
Mattias Rönnblom Oct. 18, 2023, 6:29 a.m. UTC | #7 
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
Bruce Richardson Oct. 18, 2023, 9:45 a.m. UTC | #8 
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
Bruce Richardson Oct. 18, 2023, 12:13 p.m. UTC | #9 
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
diff mbox series

Patch

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;