Minor cleanups in mmc_xpt.c
ClosedPublic
Actions

Authored by kibab on Jul 28 2020, 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.

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

kibab requested review of this revision.Jul 28 2020, 7:30 AM

This revision is now accepted and ready to land.Jul 28 2020, 1:30 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.Aug 4 2020, 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.Aug 4 2020, 10:33 AM

imp added inline comments.Aug 4 2020, 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...