linux-kernel - Re: [PATCH 1/3] drivers/misc: Add realtek card reader core driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <201208011431.02050.arnd@arndb.de>
Date:	Wed, 1 Aug 2012 14:31:01 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	wwang <wei_wang@...lsil.com.cn>
Cc:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"cjb@...top.org" <cjb@...top.org>, "bp@...en8.de" <bp@...en8.de>
Subject: Re: [PATCH 1/3] drivers/misc: Add realtek card reader core driver

On Wednesday 01 August 2012, wwang wrote:
> 于 2012年07月31日 19:23, Arnd Bergmann 写道:
> >
> > You posted the sdmmc host driver and the pci card reader driver.
> > I assume that the USB card reader and the memstick host
> > will also get posted at some point. Do you have a timeframe
> > for those?
> 
> I will submit my memstick host driver around two months later, and
> submit USB part at the end of this year.

ok.

> > The hardware seems to also support xd/smartmedia. Do you have
> > plans to add those? I think we have some code in drivers/mfd
> > that acts as an abstraction layer for these, given that the
> > host has to do the wear leveling there too.
> 
> Yes, xD is still in our plan. But because the user population of xD/SM
> is so small, we put the priority of writing xD host driver in a relevant
> low level.
> Maybe we will submit this driver in the next year.

Ok. When you get to that, I think you should use the code
from drivers/mtd/nand/sm_common.c, but it's better to confirm
that with the MTD maintainers first.

> > As usual for most things getting added to drivers/misc, I'm skeptical
> > about it actually fitting in here. Normally I'd put such a multiplexer
> > into drivers/mfd. Given that you actually need your own bus, rather
> > than just a single host with multiple endpoints, drivers/bus/realtek/cr
> > might be best.
> We do need a bus driver. We support multi models of card at the same
> time, so we need a way to bind all of the host drivers. And in the
> internal of our card reader, we have only one DMA engine and one ring
> buffer to handle massive data, so we also need a way to protect the
> critical area between different card hosts. Bus driver is convenient to
> handle this situation. Another way to fulfill is to call every register
> function of different host (like mmc, memstick) in sequence in card
> reader driver, whether pci-based or usb-based, if not using bus driver.
> I prefer the prior scheme, which is more flexible and less redundant code.

I understand where you are coming from, but IMHO a bus driver would
make more sense if the bus was a low-level abstraction that allows you
to add new high-level drivers (memstick, smartmedia, ...) without
having to modify the low-level drivers, which I believe is not possible
with your current code.

> Using bus driver:
> 
> sdmmc memstick
> \ /
> \ /
> \ /
> rtsx bus driver
> / \
> / \
> / \
> / \
> rtsx pci rtsx usb

In reality, what you have seems to be actually more like

    sdmmc    memstick
        \       /
         \     /
          \   /
     rtsx bus driver
       /         \
      /           \
     /             \
    /               \
  rtsx-pci         rtsx-usb
   /    \           /    \
 pci-mmc \       usb-mmc  \
   pci-memstick       usb-memstick

> Not using bus driver:
> 
> sdmmc-pci memstick-pci
> \ /
> \ /
> \ /
> \ /
> rtsx pci
> 
> sdmmc-usb memstick-usb
> \ /
> \ /
> \ /
> \ /
> rtsx usb
> 
> And you said we should put our own bus driver in drivers/bus/realtek/cr,
> but where is drivers/bus? Or can I just put this bus driver and our
> pci/usb card reader driver into drivers/mfd?

The best driver abstractions have the most specific code as a top-level
module that calls into more generic code.

What I would suggest you do is to have the code that is common between
the USB and PCI drivers in a loadable module that both of the other
modules call into:


   sdmmc-pci      sdmmc-usb        memstick-pci      memstick-usb
     \   \           /   \             /    \           /   |
      |   \         /     \           /      \         /    |
      |    \       /       \___      /        \       /     |
      \    sdmmc-common     ___|____/      memstick-common  |
       \     |             /   |                       |    /
        \____|______      /    |____________      _____|___/
             |      \    /                  \    /     |   
             |     pci-common             usb-common   |
             \              \              /           /
              \              \            /            /
               \_____________ \          /____________/
                             \rtsx-common/


You can skip a few of these if they are not needed, but in principle
you should get the idea. In this example, the pci-common and the
usb-common drivers would each be MFD driver that export a bunch
of slave devices. All the mmc specific code that you currently
have in the pci driver would then go all the way to the top into
the sdmmc-pci driver, and anything that is shared goes into one
of the lower modules.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/