Change in coreboot[master]: device/pci: Add NULL check for PCI driver's .ops

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Nico Huber (Code Review)

12 Oct 2020 12 Oct '20

7:35 a.m.

Hello Felix Singer,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/46297

to review the following change.

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

device/pci: Add NULL check for PCI driver's .ops

Add a NULL check and only skip setting the default operations if `.ops` was set by a driver.

Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/pci_device.c 1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/46297/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 80dad7e..62c4535 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -955,13 +955,16 @@ if ((driver->vendor == dev->vendor) && device_id_match(driver, dev->device)) { dev->ops = (struct device_operations *)driver->ops; - printk(BIOS_SPEW, "%s [%04x/%04x] %sops\n", - dev_path(dev), driver->vendor, driver->device, - (driver->ops->scan_bus ? "bus " : "")); - return; + break; } }

+ if (dev->ops) { + printk(BIOS_SPEW, "%s [%04x/%04x] %sops\n", dev_path(dev), + driver->vendor, driver->device, (driver->ops->scan_bus ? "bus " : "")); + return; + } + /* If I don't have a specific driver use the default operations. */ switch (dev->hdr_type & 0x7f) { /* Header type */ case PCI_HEADER_TYPE_NORMAL:

Attachments:

attachment.htm (text/html — 3.6 KB)

Show replies by date

Angel Pons (Code Review)

12 Oct 12 Oct

9:54 a.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46297 )

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/46297/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/46297/1//COMMIT_MSG@10 PS1, Line 10: if `.ops` was set by a driver. Maybe add:

This avoids a NULL pointer dereference in case a driver with NULL .ops would match.

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 12 Oct 2020 16:54:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

13 Oct 13 Oct

2:23 p.m.

Hello Felix Singer, build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46297

to look at the new patch set (#2).

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

device/pci: Add NULL check for PCI driver's .ops

Add a NULL check and only skip setting the default operations if `.ops` was set by a driver. It's fairly unlikely that some- body adds a driver and forgets the `.ops` pointer. So this is mostly to increase readability: Nobody should have to wonder if we're missing a NULL-check.

The condition is moved out of the loop to reduce indentation levels. Alternatively, we could jusk skip drivers that don't have `.ops` set (i.e. continue the loop).

Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/pci_device.c 1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/46297/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Angel Pons (Code Review)

3:37 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46297 )

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 13 Oct 2020 22:37:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

14 Oct 14 Oct

1:56 a.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46297 )

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46297/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/46297/1//COMMIT_MSG@10 PS1, Line 10: if `.ops` was set by a driver.

...

Maybe add: […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 14 Oct 2020 08:56:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Stefan Reinauer (Code Review)

2 Nov 2 Nov

6:42 p.m.

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46297 )

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 03 Nov 2020 02:42:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

16 Nov 16 Nov

4:15 a.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46297 )

Change subject: device/pci: Add NULL check for PCI driver's .ops ......................................................................

device/pci: Add NULL check for PCI driver's .ops

The condition is moved out of the loop to reduce indentation levels. Alternatively, we could jusk skip drivers that don't have `.ops` set (i.e. continue the loop).

Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46297 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/pci_device.c 1 file changed, 7 insertions(+), 4 deletions(-)

Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Angel Pons: Looks good to me, approved

diff --git a/src/device/pci_device.c b/src/device/pci_device.c index c3f3564..a008977 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -954,13 +954,16 @@ if ((driver->vendor == dev->vendor) && device_id_match(driver, dev->device)) { dev->ops = (struct device_operations *)driver->ops; - printk(BIOS_SPEW, "%s [%04x/%04x] %sops\n", - dev_path(dev), driver->vendor, driver->device, - (driver->ops->scan_bus ? "bus " : "")); - return; + break; } }

-- To view, visit https://review.coreboot.org/c/coreboot/+/46297 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5dcc05ebb092fb9c4be929c81ea2b05a10b1311b Gerrit-Change-Number: 46297 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1461

days inactive

1496

days old

coreboot-gerrit@coreboot.org

6 comments

4 participants

tags (0)

participants (4)

Angel Pons (Code Review)
Nico Huber (Code Review)
Patrick Georgi (Code Review)
Stefan Reinauer (Code Review)