Page MenuHomeFreeBSD

Minor cleanups in mmc_xpt.c
ClosedPublic

Authored by kibab on Tue, Jul 28, 7:30 AM.

Details

Reviewers
manu
imp
Group Reviewers
cam
Commits
rS363870: Minor cleanups in mmc_xpt.c
Summary
  • Downgrade some CAM debug messages from _INFO to _DEBUG level;
  • Add KASSERT for the case when we suspect incorrect CAM SIM initialization (using cam_sim_alloc() instead of cam_sim_alloc_dev());
  • Use waiting version of xpt_alloc_ccb(), we are not in hurry;
  • With the waiting version we cannot get NULL return, so remove the NULL check;
  • In some csses, the name of mmcprobe_done has been written as mmc_probedone();
  • Send AC_LOST_DEVICE if we, well, lost the device;
  • Misc style(9) fixes.
Test Plan

No functional changes apart from being a bit less chatty on boot now.

Diff Detail

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

Event Timeline

kibab created this revision.Tue, Jul 28, 7:30 AM
kibab requested review of this revision.Tue, Jul 28, 7:30 AM
manu accepted this revision.Tue, Jul 28, 1:30 PM
This revision is now accepted and ready to land.Tue, Jul 28, 1:30 PM
imp added a comment.Mon, Aug 3, 8:13 PM

I think this may be OK. The only real heart-burn I have is the xpt_async ...

sys/cam/mmc/mmc_xpt.c
408 ↗(On Diff #75039)

Since I'm in the area, I'd be tempted to fix this to match style(9)

412 ↗(On Diff #75039)

This races the timeout that's setup in at least dwmmc_attach. It's likely OK, though, then initialize things and then call this directly from the startup.... something to think about though...

415 ↗(On Diff #75039)

Are you allowed to block here? I didn't audit all the places this is called, but I think it's OK.

424 ↗(On Diff #75039)

I don't think this can actually ever fail.

1144 ↗(On Diff #75039)

Why do you need to do this here? Usually the SIM sends this when it detects that a periph that used to be there has departed.

kibab requested review of this revision.Tue, Aug 4, 10:32 AM
kibab marked 3 inline comments as done.
kibab added inline comments.
sys/cam/mmc/mmc_xpt.c
412 ↗(On Diff #75039)

I don't see how this KASSERT races with anything -- it's here to prevent usage of cam_sim_alloc() (without _dev()) with MMCCAM drivers since it breaks SDIO bus.

415 ↗(On Diff #75039)

there are 4 call sites for this function and none happens in the interrupt context, so I guess it's fine.

424 ↗(On Diff #75039)

Well there are some calls to malloc(...,M_NOWAIT) deep inside the call stack, so I'd check the return code just in case, it's a good practice anyway.

1144 ↗(On Diff #75039)

Because we are doing this from the mmcprobe device code (which is effectively a SIM), and getting here means that the device has disappeared.
See also ata_xpt.c:846, scsi_xpt.c:1299, it's the same thing.

kibab marked 4 inline comments as done and 2 inline comments as done.Tue, Aug 4, 10:33 AM
kibab updated this revision to Diff 75351.Tue, Aug 4, 10:45 AM
  • Fix style(9)
imp added inline comments.Tue, Aug 4, 5:21 PM
sys/cam/mmc/mmc_xpt.c
412 ↗(On Diff #75039)

It' is a good kassert, since it made me look and I found the races that I described which this KASSERT will detect on systems that are super-duper slow. Sorry if I wasn't clear about it...

424 ↗(On Diff #75039)

I agree with the check, but don't think we should report it here...

1144 ↗(On Diff #75039)

ah, so they do... I'd forgotten about that case...

kibab marked 2 inline comments as done.Tue, Aug 4, 7:58 PM
kibab updated this revision to Diff 75394.Tue, Aug 4, 7:58 PM
  • Remove excessive printf
imp accepted this revision.Tue, Aug 4, 8:00 PM
This revision is now accepted and ready to land.Tue, Aug 4, 8:00 PM
manu accepted this revision.Tue, Aug 4, 8:01 PM
This revision was automatically updated to reflect the committed changes.