camcontrol: possibly wrong sector count in ata_do_identify()
ClosedPublic
Actions

Authored by danfe on Mar 1 2019, 1:53 AM.

Details

Reviewers

smh
imp
ken
mav

Commits

rS344701: Fix incorrect / unused sector_count for identify requests

Summary

I've noticed that in the ata_do_identify() function that was added in rS249115 issues ATA_ATA_IDENTIFY (0xec) command which reads a sector (512 bytes) of drive identification data (struct ata_params), but passes (u_int8_t)sizeof(struct ata_params) as sector_count which evaluates to zero.

This looks at least bogus, if not wrong. All the similar code I've seen, including e.g. that of popular Linux hdparm utility, supply 1 (one) as a sector count in this case. Even if zero is correct, it should not be obfuscated as a cast of 512 to u_int8_t which is too large and silently gets truncated.

I've tried to contact @smh several times about the matter, but he never replied, so apparently he's no longer active with the project, thus I'm putting it up for review here.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 22815

Event Timeline

danfe created this revision.Mar 1 2019, 1:53 AM

Harbormaster completed remote builds in B22815: Diff 54563.Mar 1 2019, 1:53 AM

Sorry for lack of responses, I did see your requests but got lost in work stuff.

I've tested with zero for both standard ata and scsi ata pass through with no ill effects and as the ata spec defines the value as N/A I believe a zero is the more correct value.

If no objections I'll commit that change later today.

camcontrol.c
2295	I believe 0 is actually the correct value in both case as both lba and count are ignored by the identify command.

This revision now requires changes to proceed.Mar 1 2019, 9:23 AM

danfe added inline comments.Mar 1 2019, 2:32 PM

camcontrol.c
2295	I confirmed with the spec, it indeed specifies N/A, although I didn't find how they suggest N/A to be translated in implementations (code). That said, I'm fine with zero.

I agree, zero should be the right answer here. That is what kernel does also.

This revision was not accepted when it landed; it landed in state Needs Revision.Mar 1 2019, 2:39 PM

Closed by commit rS344701: Fix incorrect / unused sector_count for identify requests (authored by smh). · Explain Why

This revision was automatically updated to reflect the committed changes.

I figured that this was OK, but just checked this am and I noticed this commit.
Just wanted to say that it looks good to me too.
Thanks for driving this home @smh

Revision Contents
Changeset List

Path

Size

camcontrol.c

5 lines

Diff 54563

View Options

camcontrol: possibly wrong sector count in ata_do_identify()ClosedPublicActions