Re: [PATCH 3/3 v7] block: Add n64 cart driver

[Christoph Hellwig <hch@infradead.org>]

> +
> +MODULE_AUTHOR("Lauri Kasanen <cand@gmx.com>");
> +MODULE_DESCRIPTION("Driver for the N64 cart");
> +MODULE_LICENSE("GPL");
> +
> +#define BUFSIZE (64 * 1024)

Maybe use SZ_64K here?

> +	void *dst = bio_data(req->bio);
> +
> +	if (bstart + len > size)
> +		return BLK_STS_IOERR;
> +
> +	bstart += start;
> +
> +	while (len) {
> +		const u32 curlen = len < BUFSIZE ? len : BUFSIZE;
> +
> +		dma_cache_inv((unsigned long) buf, curlen);

dma_cache_inv is not a public API, but only a helper for the DMA API.

> +		n64cart_wait_dma();
> +
> +		barrier();
> +		n64cart_write_reg(PI_DRAM_REG, dma_addr);
> +		barrier();

barrier is just a compiler barrier.  But the mmio APIs already include
actual cpu barriers where applicable, so I don't think you need these at
al.

> +		n64cart_write_reg(PI_CART_REG, (bstart | 0x10000000) & 0x1FFFFFFF);

Overly long line.  Also this looks a little too magic, can you explain
what is going on here with symbolic constants and&or a helper?

> +
> +	do {
> +		err = get_seg(req);
> +	} while (blk_update_request(req, err, blk_rq_cur_bytes(req)));
> +
> +	blk_mq_end_request(req, BLK_STS_OK);

This should be __blk_mq_end_request given that you call
blk_update_request manually.

> +	major = register_blkdev(0, "n64cart");

No need to call register_blkdev in a new driver, just set the
GENHD_FL_EXT_DEVT flag in struct gendisk.

> +	buf = kmalloc(BUFSIZE, GFP_DMA | GFP_KERNEL);
> +	if (!buf) {
> +		err = -ENOMEM;
> +		goto fail_queue;
> +	}
> +	dma_addr = virt_to_phys(buf);

Please use dma_aloc_noncoherent to allocate dma memory.  And then
use the dma_sync_single_for_{device,cpu} to transfer ownership to
and from the device.


Just curious:  wouldn't it make more sense to implement this as a bio
based driver with a helper thread?  It doesn't look like blk-mq provides
you a lot of benefits here.  And then we could make the blk-mq code
optional and you could save a fair amount of a kernel memory on your
rather constrained device.