Hello Sebastian,
thanks for the review.
On 01/04/2020 10:41, Sebastian Huber wrote:
> On 01/04/2020 10:03, Christian Mauderer wrote:
>
>> The rtems_bsd_mmcsd_attach_worker acquired the bus without releasing it.
>> Fix that by only acquire the bus if necessary (during initialization and
>> during read / writes).
>> ---
>> freebsd/sys/dev/mmc/mmcsd.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/freebsd/sys/dev/mmc/mmcsd.c b/freebsd/sys/dev/mmc/mmcsd.c
>> index 077f8d74..674be6a7 100644
>> --- a/freebsd/sys/dev/mmc/mmcsd.c
>> +++ b/freebsd/sys/dev/mmc/mmcsd.c
>> @@ -268,6 +268,8 @@ rtems_bsd_mmcsd_set_block_size(device_t dev,
>> uint32_t block_size)
>> memset(&req, 0, sizeof(req));
>> memset(&cmd, 0, sizeof(cmd));
>> + MMCBUS_ACQUIRE_BUS(device_get_parent(dev), dev);
>> +
>> req.cmd = &cmd;
>> cmd.opcode = MMC_SET_BLOCKLEN;
>> cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> @@ -278,6 +280,8 @@ rtems_bsd_mmcsd_set_block_size(device_t dev,
>> uint32_t block_size)
>> status_code = RTEMS_IO_ERROR;
>> }
>> + MMCBUS_RELEASE_BUS(device_get_parent(dev), dev);
>> +
>> return status_code;
>> }
>> @@ -316,6 +320,7 @@ rtems_bsd_mmcsd_disk_read_write(struct
>> mmcsd_part *part, rtems_blkdev_request *b
>> data_flags = MMC_DATA_READ;
>> }
>> + MMCBUS_ACQUIRE_BUS(device_get_parent(dev), dev);
>> MMCSD_DISK_LOCK(part);
>
> I don't really know when you have to acquire the bus, but it is an
> expensive operation. Did you measure the performance impact of this change?
>
No I didn't measure the performance impact.
The original FreeBSD code does the acquire and release around bus
accesses in the mmcsd_task. It covers switching partitions as well as
reading and writing.:
MMCBUS_ACQUIRE_BUS(mmcbus, dev);
sz = part->disk->d_sectorsize;
block = bp->bio_pblkno;
end = bp->bio_pblkno + (bp->bio_bcount / sz);
err = mmcsd_switch_part(mmcbus, dev, sc->rca, part->type);
if (err != MMC_ERR_NONE) {
if (ppsratecheck(&sc->log_time, &sc->log_count,
LOG_PPS))
device_printf(dev, "Partition switch error\n");
goto release;
}
if (bp->bio_cmd == BIO_READ || bp->bio_cmd == BIO_WRITE) {
/* Access to the remaining erase block obsoletes it. */
if (block < part->eend && end > part->eblock)
part->eblock = part->eend = 0;
block = mmcsd_rw(part, bp);
} else if (bp->bio_cmd == BIO_DELETE) {
block = mmcsd_delete(part, bp);
}
release:
MMCBUS_RELEASE_BUS(mmcbus, dev);
> I guess the use of the disk lock here was an optimization. If we really
> want to use the bus lock, then the disk lock should be removed.
>
>> for (i = 0; i < buffer_count; ++i) {
>> @@ -394,6 +399,7 @@ rtems_bsd_mmcsd_disk_read_write(struct mmcsd_part
>> *part, rtems_blkdev_request *b
>> error:
>> MMCSD_DISK_UNLOCK(part);
>> + MMCBUS_RELEASE_BUS(device_get_parent(dev), dev);
>> rtems_blkdev_request_done(blkreq, status_code);
>> @@ -436,8 +442,6 @@ rtems_bsd_mmcsd_attach_worker(rtems_media_state
>> state, const char *src, char **d
>> goto error;
>> }
>> - MMCBUS_ACQUIRE_BUS(device_get_parent(dev), dev);
>
> I think the bus acquire here is necessary to prevent an
> rtems_blkdev_create() while the bus is detached in parallel, e.g. a fast
> plug/unplug of an USB stick.
>
> After reviewing the code I think this bus acquire without a release
> (which is in the code since the import from libusb in 2015) was a hack
> to improve the read/write performance. We didn't support hot plugging of
> MMC busses and devices. Do we need this feature now?
Sorry I didn't explain it: The problem is not hot plugging the busses or
the card. For that quite a bit more changes would be necessary. The
problem is that eMMC cards have multiple (hardware) partitions and
therefore call this code multiple times. For the second partition the
bus is already locked and therefore the MMCBUS_ACQUIRE_BUS just hangs.
The function is called from the media_server which then hangs too and
can't handle any new disks.
But I noted that there is a partition switching missing for the eMMC
cards too. Otherwise the wrong partition is used during detection of the
second one.
So another maybe better approach for now is to ignore any additional
hardware partions except the first one. That would need a hack to
mmcsd_add_part. There would be more work anyway if someone really wants
to use the second hardware partition.
Note: Hardware partitions don't have anything to do with the "normal"
disk partitions. It would be more similar to two SD cards on one bus.
It's a feature used by for example smart phones to store some stuff like
a signed kernel or some DRM keys that shouldn't be accessible in the
normal user space.
>
>> -
>> status_code = rtems_bsd_mmcsd_set_block_size(dev, block_size);
>> if (status_code != RTEMS_SUCCESSFUL) {
>> printf("OOPS: set block size failed\n");
--
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.maude...@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
