Page MenuHomeFreeBSD

ACPI Hardware ID match routine to distinguish _HID match and _CID match.
ClosedPublic

Authored by takawata on Jul 27 2018, 12:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 4:02 PM
Unknown Object (File)
Feb 23 2024, 4:01 PM
Unknown Object (File)
Feb 23 2024, 3:59 PM
Unknown Object (File)
Feb 23 2024, 3:59 PM
Unknown Object (File)
Feb 23 2024, 3:58 PM
Unknown Object (File)
Feb 23 2024, 3:58 PM
Unknown Object (File)
Feb 23 2024, 3:56 PM
Unknown Object (File)
Feb 23 2024, 3:56 PM
Subscribers
None

Details

Summary

This patch will distinguish _HID exact match and match by _CID
compatibility match. Generally _CID match is suboptimal and
now we can tell its match priority to the FreeBSD bus framwork.
It sometimes may not appropriate to attach if compatiblity match.
This changes ACPI related KPI and it needs kernel revision bump.
(It should be ACPI module revision bump, but it is not tribial so
it should be done in another changeset if we do.)

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 18369
Build 18085: arc lint + arc unit

Event Timeline

Generally, I like this, modulo the couple of issues that need to be addressed. the biggest one is the ENXIO test being too specific.

sys/dev/acpi_support/acpi_ibm.c
352

This might be unrelated.

421

style nit: there should be a space between if and (.

sys/dev/acpica/acpi.c
1186

There's a mix of styles used here. Some places you test against 0 (eg > 0 is an error) other places the exact error. I think it would be better to test <= 0 here and other places you explicitly tested against ENXIO.

I would probably suggest doing a pass over the diff to fix the style things and Warner's suggestion of comparing '< 0' instead of against ENXIO, removing goto's, etc.

I think overall the change is fine. One possible issue is that if you match on a CID before a HID you might return the wrong return value (e.g. if a device has both a _HID and a _CID that are in the list of IDs, but the _CID is first in the driver table). We could fix this by changing how the matching loop works. Rather than looping through the list of IDs in the table calling acpi_matchHid(), you would take the _HID and look for that value in the table of IDs. Then do the same for each entry in _CID. This would mean you couldn't use acpi_MatchHid() in ACPI_IDS_PROBE() anymore, but would give a more accurate bus probe value.

sys/dev/acpi_support/acpi_fujitsu.c
234

Space after '>'

236

Trailing whitespace here?

sys/dev/acpi_support/acpi_ibm.c
422

I would check the indent on this line, should be two tabs.

sys/dev/acpi_support/acpi_rapidstart.c
66

Please keep the blank line between variable declarations and the first line of code (various instances of this in the diff).

70

Space after 'if' here as well.

sys/dev/acpi_support/acpi_sony.c
119

I would follow what you've done for other drivers here. I would also break out the side effect from the if and do:

ret = ACPI_ID_PROBE(...);
if (ret < 0)
    device_set_desc(...);
return (ret);
sys/dev/acpica/acpi.c
1660

I don't think you need this assignment. I think callers can only expect 'match' to be set if the function finds a match (we have other APIs that work similarly)

1664

Perhaps 'if (rv != ACPI_MATCHHID_NOMATCH)', but you could also use 'continue' to reduce the indentation if you wanted:

rv = acpi_MatchHid(h, ids[i]);
if (rv == ACPI_MATCHHID_NOMATCH)
    continue;
if (match != NULL)
    *match = ids[i];
return (...);
sys/dev/acpica/acpi_if.m
83

Keep an empty line before 'Returns:'

sys/dev/acpica/acpi_resource.c
704

I'm not sure about keeping this check. If we do want this, this should be a separate commit as it would introduce a behavior change on its own right. However, I'm not sure we want to ignore devices that have SYSRES as a CID. We don't do that today.

sys/dev/acpica/acpi_smbat.c
116

Just use 'return' directly instead of a goto.

updated:
Thank you for your comments.
Style nits.
Remove non-related change.
Compare with 0 to check ACPI_ID_PROBE
add comment on acpi_MatchHID

(Sorry, I thought you were going to go over the patch to find other examples of style things besides the ones Warner and I had pointed out. I've gone over the whole diff now.)

sys/dev/acpi_support/acpi_panasonic.c
147–149

Space after 'if' here.

sys/dev/acpi_support/acpi_toshiba.c
224–225

Blank line here between variables and code.

sys/dev/acpi_support/acpi_wmi.c
215

You could drop this blank line perhaps.

sys/dev/acpica/acpi.c
1664

Move the space before the open paren instead of after.

1667

Space after 'if' here and also before '{'

sys/dev/acpica/acpi_button.c
105

Space after 'if'.

sys/dev/acpica/acpi_cmbat.c
124

Space after 'if'.

sys/dev/acpica/acpi_cpu.c
255

Maybe '> 0' rather than '== ENXIO'?

sys/dev/acpica/acpi_ec.c
365–366

I think for here it might be more consistent to do this:

} else {
    ret = ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids, NULL);
    if (ret > 0)
        goto out;
    ...

And then fix the 'out' label to handle return values that aren't 0.

425

This would also need to be updated to handle 'ret' to set it to ENXIO in the device_disable case, maybe by just inverting the conditional:

if (peer != NULL && device_is_alive(peer)) {
    device_disable(dev);
    ret = ENXIO;
}
433

Make this 'if (ret <= 0) {'.

sys/dev/acpica/acpi_hpet.c
458

Space after 'if'.

sys/dev/acpica/acpi_isab.c
101

Space after 'if'.

sys/dev/acpica/acpi_pci_link.c
158

return (rv) perhaps? That seems more consistent with your other changes.

sys/dev/acpica/acpi_resource.c
704

return (rv)?

sys/dev/acpica/acpi_smbat.c
115

Space after 'if'.

116

return (rv)?

sys/dev/asmc/asmc.c
380–382

Could do return (rv) here.

sys/dev/fdc/fdc_acpi.c
86

Space after 'if'.

86–88

return (rv)?

sys/dev/gpio/bytgpio.c
547

Space after 'if'

sys/dev/gpio/chvgpio.c
351

Space after 'if'

sys/dev/hyperv/vmbus/vmbus_res.c
82

Space after 'if'

sys/dev/ichiic/ig4_acpi.c
74

Space after 'if'

sys/dev/intel/spi.c
431

Space after 'if'

takawata marked 31 inline comments as done.

After freeze I update this patch.
Style nit, amdgpio rewrite is added.

Update acpi_ec , button style nit.

takawata marked 8 inline comments as done.

acpi_cpu nit.

This revision is now accepted and ready to land.Oct 25 2018, 5:38 PM

This looks good to me as well...

This revision was automatically updated to reflect the committed changes.