run_mmc_cmd in the cdio_func_t should be left alone for backwards compatibility and because it implements a well-defined concept defined (if incomplete by itself) function of the MMC standard.
Again, we should not contemplate changing something that exists and that people might be using for something proposed however better, but might need adjustment and adoptance time. It would be fine to create a function that runs a mmc command and then issues a sense command. Since it issues the mmc command it knows what command it ran. Things that bundle MMC commands are put in mmc_hl.c . If this is not clear, look at mmc_eject_media which allows media removal before issuing, mmc_start_stop_unit. Or mmc_mode_sense which can issue two mode sense commands. If there are reasonable function that only need to issue mode_sense commands under certain conditions, then again that goes into mmc_hl.c. On Wed, Jun 8, 2016 at 11:34 AM, Thomas Schmitt <[email protected]> wrote: > Hi, > > when testing the sense code evaluator and its messages i got to a > problem of layering and the encapsulated knowledge about the performed > SCSI command (mmc_cdb_t). > I see the following alternatives: > > - Move sense code evaluation and reporting to functions which know > the mmc_cdb_t. This knowledge is currently quite low-level and very > volatile. > > - Bottleneck all calls to p_cdio->op.run_mmc_cmd() through MMC_RUN_CMD() > which would then record the mmc_cdb_t before the command gets > executed. > > - Put responsibility for recording mmc_cdb_t on each implementation > of p_cdio->op.run_mmc_cmd(). This is already done for sense data, > because they have to be copied from driver specific memory to > generic_img_private_t. > > -------------------------------------------------------------------- > Reasoning: > > The sense code messages are incomplete if they are not accompanied > by info about the SCSI command which got them as reply. > But the functions which could best judge the impact of sense codes > are too high in the layer stack to know the SCSI command CDBs. > > So additionally to the last received sense data, generic_img_private_t > should get a buffer to record the SCSI command which caused the buffered > sense data. > > typedef struct { > ... > mmc_cdb_t scsi_mmc_cmd; > int scsi_mmc_cmd_len; > ... > } generic_img_private_t; > > These new struct members could be filled by MMC_RUN_CMD() which knows > them as cdb and mmc_get_cmd_len(cdb.field[0]). > > Regrettably not all SCSI command transactions are performed via > MMC_RUN_CMD(). There are direct uses of p_cdio->op.run_mmc_cmd() > and even function pointers handed down to other functions. > > How many implementations of SCSI command transport do we have ? > > -------------------------------------------------------------------- > > The problem is partly hidden by run_mmc_cmd_linux(), because with severe > sense codes, the ioctl throws an error and run_mmc_cmd_linux() reports > > INFO: ioctl CDROM_SEND_PACKET for command READ_SUBCHANNEL (0x42) failed: > Input/output error > > before mmc_get_mcn_isrc_private() can call mmc_evaluate_last_sense() > which then says > > INFO: [5 24 00] : Illegal Request, Invalid field in cdb > > I am quite sure that ioctl(SG_IO) would not throw a Unix error in > the same situation. We cannot rely on the SCSI command name to be yelled > by the SCSI transport layer when interesting sense data arrive. > > ioctl(CDROM_SEND_PACKET) will not throw an error command name if a harmless > sense code is encountered. But we still might want to report it. Possibly > via cdio_error(). > > -------------------------------------------------------------------- > > Have a nice day :) > > Thomas > > >
