Skip to content

Conversation

nikhalim
Copy link

Check whether parent device is of the anon_device_type before getting its platform_device

@nikhalim
Copy link
Author

@sjasonsmith @gratian Please help to review this PR

Copy link
Collaborator

@sjasonsmith sjasonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this? Ideally you would test both that it fixes the crashing you are seeing when pciserial unloads, as well as that it continues to work through surprise removal sequences with cRIO.

struct serial_port_device *port_dev = port->port_dev;
struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
int ctrl_id = port->ctrl_id;
struct platform_device *pdev;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not using this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@sjasonsmith
Copy link
Collaborator

@gratian this technique was derived from this old post where a similar issue was resolved elsewhere.
https://fa.linux.kernel.narkive.com/3rmBMJOU/patch-v2-mfd-only-unregister-platform-devices-allocated-by-the-mfd-core

serial_base_ctrl_device_remove(new_ctrl_dev);

if (strncmp(to_platform_device(port->dev)->name, "anon_port", 9) == 0)
if (is_parent_null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Seeing that if the parent is not NULL anymore after port->dev = &pdev->dev; above, naming this is_parent_anon_port or something like that might be better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link

@gratian gratian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks good. In addition to the improvements suggested @sjasonsmith, and @chaitu236 the commit message could use some tweaking so that in the future we know why the change was needed. It would also let us know that it should be combined with the commit it fixes on the next rebase (it also needs a 'Fixes' tag).

I would reword it to say something like:

Commit 978ca75850f2 ("serial: core: create anonymous parent device")
introduced a NULL pointer dereference by accessing ...

Check whether parent device is of the anon_device_type before getting
its platform_device.

Fixes: 978ca75850f2 ("serial: core: create anonymous parent device")
Signed-off-by: <your signoff>

Commit 978ca75 ("serial: core: create anonymous
parent device") introduced a NULL pointer dereference
when attempting to access the platform_device structure
of the parent.

This commit adds a check for parent's device type before
getting its platform_device structure.

Fixes: 978ca75 ("serial: core: create anonymous parent device")
Signed-off-by: Kevin Lim <[email protected]>
@nikhalim nikhalim force-pushed the nikhalim-patch-serialcore branch from 731a940 to 0b1452d Compare March 13, 2025 01:10
@nikhalim
Copy link
Author

All feedbacks addressed.

./scripts/checkpatch.pl 0001-serial-core-add-checking-for-platform-device.patch
total: 0 errors, 0 warnings, 41 lines checked

0001-serial-core-add-checking-for-platform-device.patch has no obvious style problems and is ready for submission.

@gratian gratian merged commit 5238c99 into ni:nilrt/master/6.6 Mar 13, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants