Page MenuHomeFreeBSD

Minor cleanups in mmc_xpt.c

Authored by kibab on Jul 28 2020, 7:30 AM.
Referenced Files
Unknown Object (File)
Mon, Jul 8, 3:46 AM
Unknown Object (File)
Sun, Jul 7, 10:10 PM
Unknown Object (File)
Thu, Jun 27, 12:37 AM
Unknown Object (File)
Thu, Jun 27, 12:30 AM
Unknown Object (File)
Wed, Jun 26, 12:42 AM
Unknown Object (File)
Mon, Jun 24, 10:59 PM
Unknown Object (File)
Sun, Jun 23, 8:02 AM
Unknown Object (File)
Sat, Jun 22, 1:44 AM


Group Reviewers
rS363870: Minor cleanups in mmc_xpt.c
  • 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

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

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

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.
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
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.Aug 4 2020, 7:58 PM
  • Remove excessive printf
This revision is now accepted and ready to land.Aug 4 2020, 8:00 PM
This revision was automatically updated to reflect the committed changes.