[PATCH libbsd] mmcsd: Fix missing MMCBUS_RELEASE_BUS.

[Sebastian Huber]

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?

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?

> -
>   		status_code = rtems_bsd_mmcsd_set_block_size(dev, block_size);
>   		if (status_code != RTEMS_SUCCESSFUL) {
>   			printf("OOPS: set block size failed\n");