Page MenuHomeFreeBSD

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

Authored by takawata on Jul 27 2018, 12:35 PM.



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

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

takawata created this revision.Jul 27 2018, 12:35 PM
imp added a comment.Jul 27 2018, 4:13 PM

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

352 ↗(On Diff #45906)

This might be unrelated.

421 ↗(On Diff #45906)

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

1186 ↗(On Diff #45906)

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.

jhb added a comment.Jul 27 2018, 5:10 PM

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.

234 ↗(On Diff #45906)

Space after '>'

236 ↗(On Diff #45906)

Trailing whitespace here?

422 ↗(On Diff #45906)

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

66 ↗(On Diff #45906)

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

70 ↗(On Diff #45906)

Space after 'if' here as well.

119 ↗(On Diff #45906)

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)
return (ret);
1660 ↗(On Diff #45906)

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 ↗(On Diff #45906)

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 (match != NULL)
    *match = ids[i];
return (...);
83 ↗(On Diff #45906)

Keep an empty line before 'Returns:'

704 ↗(On Diff #45906)

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.

116 ↗(On Diff #45906)

Just use 'return' directly instead of a goto.

takawata updated this revision to Diff 45939.Jul 28 2018, 3:11 AM

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

jhb added a comment.Jul 30 2018, 6:15 PM

(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.)

147 ↗(On Diff #45939)

Space after 'if' here.

224 ↗(On Diff #45939)

Blank line here between variables and code.

216 ↗(On Diff #45939)

You could drop this blank line perhaps.

1662 ↗(On Diff #45939)

Move the space before the open paren instead of after.

1665 ↗(On Diff #45939)

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

106 ↗(On Diff #45939)

Space after 'if'.

124 ↗(On Diff #45939)

Space after 'if'.

255 ↗(On Diff #45939)

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

365 ↗(On Diff #45939)

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 ↗(On Diff #45939)

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)) {
    ret = ENXIO;
433 ↗(On Diff #45939)

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

459 ↗(On Diff #45939)

Space after 'if'.

101 ↗(On Diff #45939)

Space after 'if'.

159 ↗(On Diff #45939)

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

705 ↗(On Diff #45939)

return (rv)?

115 ↗(On Diff #45939)

Space after 'if'.

116 ↗(On Diff #45939)

return (rv)?

383 ↗(On Diff #45939)

Could do return (rv) here.

88 ↗(On Diff #45939)

Space after 'if'.

89 ↗(On Diff #45939)

return (rv)?

549 ↗(On Diff #45939)

Space after 'if'

351 ↗(On Diff #45939)

Space after 'if'

82 ↗(On Diff #45939)

Space after 'if'

76 ↗(On Diff #45939)

Space after 'if'

431 ↗(On Diff #45939)

Space after 'if'

takawata updated this revision to Diff 49562.Oct 24 2018, 4:39 PM
takawata marked 31 inline comments as done.

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

takawata updated this revision to Diff 49563.Oct 24 2018, 4:41 PM

Remove unrelated diff.

takawata updated this revision to Diff 49585.Oct 25 2018, 12:16 AM

Update acpi_ec , button style nit.

takawata updated this revision to Diff 49587.Oct 25 2018, 12:27 AM
takawata marked 8 inline comments as done.

acpi_cpu nit.

takawata updated this revision to Diff 49588.Oct 25 2018, 12:34 AM

Remove unrerlated diff.

takawata updated this revision to Diff 49589.Oct 25 2018, 12:35 AM

Reduce diff.

takawata updated this revision to Diff 49590.Oct 25 2018, 12:37 AM

reduce diff.

jhb accepted this revision.Oct 25 2018, 5:38 PM
This revision is now accepted and ready to land.Oct 25 2018, 5:38 PM
imp accepted this revision.Oct 25 2018, 6:46 PM

This looks good to me as well...

This revision was automatically updated to reflect the committed changes.