Initial commit for Sony MemoryStick support
PATCH 1/1 Initial commit for Sony MemoryStick support
From: Alex Dubov <[email protected]>
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
--- MAINTAINERS | 7 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/memstick/Kconfig | 26 + drivers/memstick/Makefile | 11 + drivers/memstick/core/Kconfig | 26 + drivers/memstick/core/Makefile | 11 + drivers/memstick/core/memstick.c | 557 ++++++++++++++ drivers/memstick/core/mspro_block.c | 1376 +++++++++++++++++++++++++++++++++++ drivers/memstick/host/Kconfig | 22 + drivers/memstick/host/Makefile | 10 + drivers/memstick/host/tifm_ms.c | 684 +++++++++++++++++ drivers/misc/tifm_7xx1.c | 17 + drivers/misc/tifm_core.c | 7 + include/linux/memstick.h | 289 ++++++++ include/linux/tifm.h | 4 + 16 files changed, 3050 insertions(+), 0 deletions(-) create mode 100644 drivers/memstick/Kconfig create mode 100644 drivers/memstick/Makefile create mode 100644 drivers/memstick/core/Kconfig create mode 100644 drivers/memstick/core/Makefile create mode 100644 drivers/memstick/core/memstick.c create mode 100644 drivers/memstick/core/mspro_block.c create mode 100644 drivers/memstick/host/Kconfig create mode 100644 drivers/memstick/host/Makefile create mode 100644 drivers/memstick/host/tifm_ms.c create mode 100644 include/linux/memstick.h
MAINTAINERS +7 -0
@@ -3505,6 +3505,13 @@ L: [email protected] W: http://www.linux.it/~malattia/wiki/index.php/Sony_drivers S: Maintained +SONY MEMORYSTICK CARD SUPPORT +P: Alex Dubov +M: [email protected] +L: [email protected] +W: http://tifmxx.berlios.de/ +S: Maintained + SOUND P: Jaroslav Kysela M: [email protected]
drivers/Kconfig +2 -0
@@ -76,6 +76,8 @@ source "drivers/usb/Kconfig" source "drivers/mmc/Kconfig" +source "drivers/memstick/Kconfig" + source "drivers/leds/Kconfig" source "drivers/infiniband/Kconfig"
drivers/Makefile +1 -0
@@ -77,6 +77,7 @@ obj-$(CONFIG_LGUEST_GUEST) += lguest/ obj-$(CONFIG_CPU_FREQ) += cpufreq/ obj-$(CONFIG_CPU_IDLE) += cpuidle/ obj-$(CONFIG_MMC) += mmc/ +obj-$(CONFIG_MEMSTICK) += memstick/ obj-$(CONFIG_NEW_LEDS) += leds/ obj-$(CONFIG_INFINIBAND) += infiniband/ obj-$(CONFIG_SGI_SN) += sn/
drivers/memstick/Kconfig new file +26 -0
@@ -0,0 +1,26 @@ +# +# MemoryStick subsystem configuration +# + +menuconfig MEMSTICK + tristate "Sony MemoryStick card support (EXPERIMENTAL)" + help + Sony MemoryStick is a proprietary storage/extension card protocol. + + If you want MemoryStick support, you should say Y here and also + to the specific driver for your MMC interface. + +if MEMSTICK + +config MEMSTICK_DEBUG + bool "MemoryStick debugging" + help + This is an option for use by developers; most people should + say N here. This enables MemoryStick core and driver debugging. + + +source "drivers/memstick/core/Kconfig" + +source "drivers/memstick/host/Kconfig" + +endif # MEMSTICK
drivers/memstick/Makefile new file +11 -0
@@ -0,0 +1,11 @@ +# +# Makefile for the kernel MemoryStick device drivers. +# + +ifeq ($(CONFIG_MEMSTICK_DEBUG),y) + EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_MEMSTICK) += core/ +obj-$(CONFIG_MEMSTICK) += host/ +
drivers/memstick/core/Kconfig new file +26 -0
@@ -0,0 +1,26 @@ +# +# MemoryStick core configuration +# + +comment "MemoryStick drivers" + +config MEMSTICK_UNSAFE_RESUME + bool "Allow unsafe resume (DANGEROUS)" + help + If you say Y here, the MemoryStick layer will assume that all + cards stayed in their respective slots during the suspend. The + normal behaviour is to remove them at suspend and + redetecting them at resume. Breaking this assumption will + in most cases result in data corruption. + + This option is usually just for embedded systems which use + a MemoryStick card for rootfs. Most people should say N here. + +config MSPRO_BLOCK + tristate "MemoryStick Pro block device driver" + depends on BLOCK + help + Say Y here to enable the MemoryStick Pro block device driver + support. This provides a block device driver, which you can use + to mount the filesystem. Almost everyone wishing MemoryStick + support should say Y or M here.
drivers/memstick/core/Makefile new file +11 -0
@@ -0,0 +1,11 @@ +# +# Makefile for the kernel MemoryStick core. +# + +ifeq ($(CONFIG_MEMSTICK_DEBUG),y) + EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_MEMSTICK) += memstick.o + +obj-$(CONFIG_MSPRO_BLOCK) += mspro_block.o
drivers/memstick/core/memstick.c new file +557 -0
557 lines not on this page · raw message
drivers/memstick/core/mspro_block.c new file +1376 -0
1,376 lines not on this page · raw message
drivers/memstick/host/Kconfig new file +22 -0
@@ -0,0 +1,22 @@ +# +# MemoryStick host controller drivers +# + +comment "MemoryStick Host Controller Drivers" + +config MEMSTICK_TIFM_MS + tristate "TI Flash Media MemoryStick Interface support (EXPERIMENTAL)" + depends on EXPERIMENTAL && PCI + select TIFM_CORE + help + Say Y here if you want to be able to access MemoryStick cards with + the Texas Instruments(R) Flash Media card reader, found in many + laptops. + This option 'selects' (turns on, enables) 'TIFM_CORE', but you + probably also need appropriate card reader host adapter, such as + 'Misc devices: TI Flash Media PCI74xx/PCI76xx host adapter support + (TIFM_7XX1)'. + + To compile this driver as a module, choose M here: the + module will be called tifm_ms. +
drivers/memstick/host/Makefile new file +10 -0
@@ -0,0 +1,10 @@ +# +# Makefile for MemoryStick host controller drivers +# + +ifeq ($(CONFIG_MEMSTICK_DEBUG),y) + EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_MEMSTICK_TIFM_MS) += tifm_ms.o +
drivers/memstick/host/tifm_ms.c new file +684 -0
684 lines not on this page · raw message
drivers/misc/tifm_7xx1.c +17 -0
@@ -302,6 +302,21 @@ static int tifm_7xx1_resume(struct pci_dev *dev)#endif /* CONFIG_PM */ +static int tifm_7xx1_dummy_has_ms_pif (struct tifm_adapter *fm, +struct tifm_dev *sock) +{ +return 0 ; +} + +static int tifm_7xx1_has_ms_pif (struct tifm_adapter *fm,struct tifm_dev *sock) +{ +if (((fm->num_sockets ==4 ) && (sock->socket_id ==2 )) + || ((fm->num_sockets ==2 ) && (sock->socket_id ==0 ))) +return 1 ; + +return 0 ; +} +static int tifm_7xx1_probe (struct pci_dev *dev,const struct pci_device_id *dev_id) {
@@ -336,6 +351,7 @@ static int tifm_7xx1_probe(struct pci_dev *dev,INIT_WORK (&fm->media_switcher, tifm_7xx1_switch_media); fm->eject = tifm_7xx1_eject; + fm->has_ms_pif = tifm_7xx1_has_ms_pif;pci_set_drvdata (dev, fm); fm->addr = ioremap(pci_resource_start(dev, 0),
@@ -377,6 +393,7 @@ static void tifm_7xx1_remove(struct pci_dev *dev)int cnt; fm->eject = tifm_7xx1_dummy_eject; + fm->has_ms_pif = tifm_7xx1_dummy_has_ms_pif;writel (TIFM_IRQ_SETALL , fm->addr +FM_CLEAR_INTERRUPT_ENABLE );mmiowb ();free_irq (dev->irq, fm);
drivers/misc/tifm_core.c +7 -0
@@ -284,6 +284,13 @@ void tifm_eject(struct tifm_dev *sock) }EXPORT_SYMBOL (tifm_eject); +int tifm_has_ms_pif (struct tifm_dev *sock) +{ +struct tifm_adapter *fm =dev_get_drvdata (sock->dev.parent); +return fm->has_ms_pif (fm, sock); +} +EXPORT_SYMBOL (tifm_has_ms_pif); +int tifm_map_sg (struct tifm_dev *sock,struct scatterlist *sg,int nents,int direction) {
include/linux/memstick.h new file +289 -0
201 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
1.5.3.6
Part of the diff is not shown here. View the raw message.
Quoted 7 lines · On Wed, 2 Jan 2008 17:42:24 +1100 [email protected] wrote:
From: Alex Dubov <[email protected]>
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
Will you be running a git tree for this? If so, please send me the link.
Will this enable that thus-far useless slot on my Vaio?
Where did the info come from which enabled this driver to be written? I thought Sony were super-secretive about this stuff?
drivers/memstick/Kconfig line 11
Quoted 12 lines
drivers/memstick/Kconfig new file +11
-0@@ -0,0 +1,26 @@ +# +# MemoryStick subsystem configuration +# + +menuconfig MEMSTICK + tristate "Sony MemoryStick card support (EXPERIMENTAL)" + help + Sony MemoryStick is a proprietary storage/extension card protocol. + + If you want MemoryStick support, you should say Y here and also + to the specific driver for your MMC interface.
Are you sure this has enough dependencies? CONFIG_TIFM_* for a start?
<fiddles with Kconfig>
We can create a config which has CONFIG_TIFM_CORE=m, CONFIG_MEMSTICK=y. That might not work?
Anyway, please have a think about that.
drivers/memstick/core/memstick.c line 72
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+static int memstick_uevent (struct device *dev,struct kobj_uevent_env *env) +{ + struct memstick_dev *card = container_of(dev, struct memstick_dev, + dev); + +if (add_uevent_var (env,"MEMSTICK_TYPE=%02X" , card->id.type)) +return -ENOMEM ; + +if (add_uevent_var (env,"MEMSTICK_CATEGORY=%02X" , card->id.category)) +return -ENOMEM ;
I trust higher-level code cleans up the MEMSTICK_TYPE string if this call fails.
Quoted 14 lines
drivers/memstick/core/memstick.c new file +14
-0+if (add_uevent_var (env,"MEMSTICK_CLASS=%02X" , card->id.class)) +return -ENOMEM ; + +return 0 ; +} + +static int memstick_device_probe (struct device *dev) +{ + struct memstick_dev *card = container_of(dev, struct memstick_dev, + dev); +struct memstick_driver *drv =container_of (dev->driver, + struct memstick_driver, + driver); +int rc = -ENODEV ;
drivers/memstick/core/memstick.c line 97
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+ +get_device (dev); +if (dev->driver && drv->probe) { + rc = drv->probe (card); +if (!rc) +return 0 ; + } +put_device (dev); +return rc; +}
Could just do:
int rc = -ENODEV;
if (dev->driver && drv->probe) {
rc = drv->probe(card);
if (!rc)
get_device(dev);
}
return rc;If the earlier get_device() is indeed needed then we already have a problem..
drivers/memstick/core/memstick.c line 109
Quoted 12 lines
drivers/memstick/core/memstick.c new file +12
-0+ +static int memstick_device_remove (struct device *dev) +{ + struct memstick_dev *card = container_of(dev, struct memstick_dev, + dev); +struct memstick_driver *drv =container_of (dev->driver, + struct memstick_driver, + driver); + +if (dev->driver && drv->remove) { + drv->remove (card); + card->dev.driver =NULL ;
Is this needed?
Quoted 21 lines
drivers/memstick/core/memstick.c new file +18
-0+ } + +put_device (dev); +return 0 ; +} +void memstick_init_req (struct memstick_request *mrq,unsigned char tpc, +void *buf,size_t length) +{ + mrq->tpc = tpc; +if (tpc &8 ) + mrq->data_dir =WRITE ; +else + mrq->data_dir =READ ; + + mrq->data_len = length >sizeof (mrq->data) ?sizeof (mrq->data) : length; +if (mrq->data_dir ==WRITE ) +memcpy (mrq->data, buf, mrq->data_len); +
drivers/memstick/core/memstick.c line 286
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+ mrq->io_type =MEMSTICK_IO_VAL ; + +if (tpc ==MS_TPC_SET_CMD || tpc ==MS_TPC_EX_SET_CMD ) + mrq->need_card_int =1 ; +else + mrq->need_card_int =0 ; + + mrq->get_int_reg =0 ; +} +EXPORT_SYMBOL (memstick_init_req);
It's kinda polite to add some commentary to global, exported-to-modules functions.
It leads to more effective code review too. Reviewing code is the process of determining whether implementation != intention. But without comments, the reviewer's starting point is intention = implementation. So we end up incorrectly returning true ;)
Quoted 14 lines
drivers/memstick/core/memstick.c new file +14
-0+static int h_memstick_read_dev_id (struct memstick_dev *card, +struct memstick_request **mrq) +{ +struct ms_id_register id_reg; + +if (!(*mrq)) { +memstick_init_req (&card->current_mrq,MS_TPC_READ_REG ,NULL , +sizeof (struct ms_id_register )); + *mrq = &card->current_mrq; +return 0 ; + }else { +if (!(*mrq)->error) { +memcpy (&id_reg, (*mrq)->data,sizeof (id_reg)); + card->id = (struct memstick_device_id){
drivers/memstick/core/memstick.c line 311
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+ .match_flags = MEMSTICK_MATCH_ALL, + .type = id_reg.type, + .category = id_reg.category, + .class = id_reg.class + }; + } +complete (&card->mrq_complete); +return -EAGAIN ; + } +}
Without comments this little reader is mystified as to why this function would return -EAGAIN, and what such a thing means.
Quoted 37 lines
drivers/memstick/core/memstick.c new file +37
-0+static int h_memstick_set_rw_addr (struct memstick_dev *card, +struct memstick_request **mrq) +{ +if (!(*mrq)) { +memstick_init_req (&card->current_mrq,MS_TPC_SET_RW_REG_ADRS , + (char *)&card->reg_addr, +sizeof (card->reg_addr)); + *mrq = &card->current_mrq; +return 0 ; + }else { +complete (&card->mrq_complete); +return -EAGAIN ; + } +} + +int memstick_set_rw_addr (struct memstick_dev *card) +{ + card->next_request = h_memstick_set_rw_addr; +memstick_new_req (card->host); +wait_for_completion (&card->mrq_complete); + +return card->current_mrq.error; +} +EXPORT_SYMBOL (memstick_set_rw_addr); + +static struct memstick_dev *memstick_alloc_card (struct memstick_host *host) +{ +struct memstick_dev *card =kzalloc (sizeof (struct memstick_dev ), +GFP_KERNEL ); +struct memstick_dev *old_card = host->card; +struct ms_id_register id_reg; + +if (card) { + card->host = host; +snprintf (card->dev.bus_id,sizeof (card->dev.bus_id), +"%s" , host->cdev.class_id); + card->dev.parent = host->cdev.dev;
drivers/memstick/core/memstick.c line 359
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+ card->dev.bus = &memstick_bus_type; + card->dev.release = memstick_free_card; + card->check = memstick_dummy_check; + + card->reg_addr = (struct ms_register_addr ){ + offsetof(struct ms_register , id), +sizeof (id_reg), + offsetof(struct ms_register , id), +sizeof (id_reg) + };
You may find that gcc generates crappy code for this: assembles a struct on the stack them does a memcpy (or even a memb-er-by-member copy). Doing member-at-a-time assignment would produce a better result. If you care much.
Quoted 37 lines
drivers/memstick/core/memstick.c new file +34
-0+init_completion (&card->mrq_complete); + + host->card = card; +if (memstick_set_rw_addr (card)) + goto err_out; + + card->next_request = h_memstick_read_dev_id; +memstick_new_req (host); +wait_for_completion (&card->mrq_complete); + +if (card->current_mrq.error) + goto err_out; + } + host->card = old_card; +return card; +err_out: + host->card = old_card; +kfree (card); +return NULL ; +} + + +struct mspro_sys_attr { +size_t size; +unsigned char *data; +unsigned char id; +char name[32 ]; +struct device_attribute sys_attr; +}; + +struct mspro_attr_entry { +unsigned int address; +unsigned int size; +unsigned char id;
drivers/memstick/core/memstick.c
Quoted 10 lines
drivers/memstick/core/memstick.c new file +10
-0+unsigned char reserved[3 ]; +} __attribute__((packed)); + +struct mspro_attribute { +unsigned short signature; +unsigned short version; +unsigned char count; +unsigned char reserved[11 ]; +struct mspro_attr_entry entries[]; +} __attribute__((packed));
Why are these packed? Do they map onto something whcih hardware knows about?
Again, the lack of comments leaves me at a loss.
Quoted 12 lines
...
drivers/memstick/core/mspro_block.c new file +9
-0+struct mspro_block_data { +struct memstick_dev *card; +unsigned int usage_count; +struct gendisk *disk; +struct request_queue *queue; +spinlock_t q_lock; +wait_queue_head_t q_wait; +struct task_struct *q_thread; +
drivers/memstick/core/mspro_block.c line 141
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+unsigned short page_size; +unsigned short cylinders; +unsigned short heads; +unsigned short sectors_per_track; + +unsigned char system; + unsigned char read_only:1, + active:1, + has_request:1, + data_dir:1;
You're aware that these four fields occupy the same machine word and that the compiler provides no locking? So if one thread attempts to modify "read_only" while another modifies "has_request", one can corrupt the work of the other?
If this is indeed safe then a comment here whcih clearly explains that would be useful.
Quoted 24 lines
drivers/memstick/core/mspro_block.c new file +21
-0+unsigned char transfer_cmd; + +int (*mrq_handler)(struct memstick_dev *card, +struct memstick_request **mrq); + +unsigned char attr_count; +struct mspro_sys_attr *attributes; + +struct scatterlist req_sg[MSPRO_BLOCK_MAX_SEGS ]; +unsigned int seg_count; +unsigned int current_seg; +unsigned short current_page; +}; +static ssize_t mspro_block_attr_show_default (struct device *dev, +struct device_attribute *attr, +char *buffer) +{ +struct mspro_sys_attr *x_attr =container_of (attr, + struct mspro_sys_attr, + sys_attr); +
drivers/memstick/core/mspro_block.c
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ssize_t cnt, rc =0 ; + +for (cnt =0 ; cnt < x_attr->size; cnt++) { +if (cnt && !(cnt %16 )) { +if (PAGE_SIZE - rc) + buffer[rc++] ='\n' ; + } + + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"%02x " , + x_attr->data[cnt]);
scnprintf, I suspect?
drivers/memstick/core/mspro_block.c line 289
Quoted 12 lines
drivers/memstick/core/mspro_block.c new file +12
-0+ } +return rc; +} + +static ssize_t mspro_block_attr_show_sysinfo (struct device *dev, +struct device_attribute *attr, +char *buffer) +{ +struct mspro_sys_attr *x_attr =container_of (attr, + struct mspro_sys_attr, + sys_attr); +struct mspro_sys_info *x_sys = (struct mspro_sys_info *)x_attr->data;
Maybe .data should have been void*.
Quoted 46 lines
drivers/memstick/core/mspro_block.c new file +46
-0+ssize_t rc =0 ; + + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"class: %x\n" , + x_sys->class); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"block size: %x\n" , +be16_to_cpu (x_sys->block_size)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"block count: %x\n" , +be16_to_cpu (x_sys->block_count)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"user block count: %x\n" , +be16_to_cpu (x_sys->user_block_count)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"page size: %x\n" , +be16_to_cpu (x_sys->page_size)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"assembly date: " +"%d %04u-%02u-%02u %02u:%02u:%02u\n" , + x_sys->assembly_date[0 ], +be16_to_cpu (*(unsigned short *)&x_sys->assembly_date[1 ]), + x_sys->assembly_date[3 ], x_sys->assembly_date[4 ], + x_sys->assembly_date[5 ], x_sys->assembly_date[6 ], + x_sys->assembly_date[7 ]); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"serial number: %x\n" , +be32_to_cpu (x_sys->serial_number)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"assembly maker code: %x\n" , + x_sys->assembly_maker_code); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"assembly model code: " +"%02x%02x%02x\n" , x_sys->assembly_model_code[0 ], + x_sys->assembly_model_code[1 ], + x_sys->assembly_model_code[2 ]); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"memory maker code: %x\n" , +be16_to_cpu (x_sys->memory_maker_code)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"memory model code: %x\n" , +be16_to_cpu (x_sys->memory_model_code)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"vcc: %x\n" , + x_sys->vcc); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"vpp: %x\n" , + x_sys->vpp); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"controller number: %x\n" , +be16_to_cpu (x_sys->controller_number)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"controller function: %x\n" , +be16_to_cpu (x_sys->controller_function)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"start sector: %x\n" , +be16_to_cpu (x_sys->start_sector)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"unit size: %x\n" , +be16_to_cpu (x_sys->unit_size)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"sub class: %x\n" , + x_sys->ms_sub_class); + rc += snprintf(buffer + rc, PAGE_SIZE - rc, "interface type: %x\n",
drivers/memstick/core/mspro_block.c line 345
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ x_sys->interface_type); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"controller code: %x\n" , +be16_to_cpu (x_sys->controller_code)); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"format type: %x\n" , + x_sys->format_type); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"device type: %x\n" , + x_sys->device_type); + rc +=snprintf (buffer + rc,PAGE_SIZE - rc,"mspro id: %s\n" , + x_sys->mspro_id); +return rc;
Again, this will return a wrong result if the output was truncated. scnprintf would fix.
Quoted 17 lines
drivers/memstick/core/mspro_block.c new file +14
-0+} + +static ssize_t mspro_block_attr_show_modelname (struct device *dev, +struct device_attribute *attr, +char *buffer) +{ +struct mspro_sys_attr *x_attr =container_of (attr, + struct mspro_sys_attr, + sys_attr); + +return snprintf (buffer,PAGE_SIZE ,"%s" , x_attr->data); +} +static int mspro_block_sysfs_register (struct memstick_dev *card) +{
drivers/memstick/core/mspro_block.c line 443
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+struct mspro_block_data *msb =memstick_get_drvdata (card); +int cnt, rc =0 ; + +for (cnt =0 ; cnt < msb->attr_count; cnt++) { + rc =device_create_file (&card->dev, + &msb->attributes[cnt].sys_attr); + +if (rc) { +if (cnt) { +for (cnt--; cnt >=0 ; cnt--)
ow. my brain hurts.
The if (cnt)' is actually unneeded. But if it were mine I'd be doing something simpler and obviouser here. Like for (i = 0; i < cnt; i++)' simple == good. !simple == !.
drivers/memstick/core/mspro_block.c line 452
Quoted 9 lines
drivers/memstick/core/mspro_block.c new file +9
-0+device_remove_file (&card->dev, + &msb->attributes[cnt] + .sys_attr); + } +break ; + } + } +return rc; +}
Could we be using attribute groups for all this?
Quoted 12 lines
drivers/memstick/core/mspro_block.c new file +9
-0+static void mspro_block_sysfs_unregister (struct memstick_dev *card) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); +int cnt; + +for (cnt =0 ; cnt < msb->attr_count; cnt++) +device_remove_file (&card->dev, &msb->attributes[cnt].sys_attr); +} +static int h_mspro_block_get_ro(struct memstick_dev *card,
drivers/memstick/core/mspro_block.c line 495
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ struct memstick_request **mrq) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); + +if ((*mrq)->error) { +complete (&card->mrq_complete); +return (*mrq)->error; + } + + if ((*mrq)->data[offsetof(struct ms_status_register, status0)]
It would be simpler and clearer to do
struct ms_status_register *msr; msr = (struct ms_status_register *)(*mrq)->data; if (msr->status0)
no?
Again, making .data void* would improve things here.
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ & MEMSTICK_STATUS0_WP) + msb->read_only =1 ; + else + msb->read_only = 0; + +complete (&card->mrq_complete); +return -EAGAIN ; +} + +static int h_mspro_block_wait_for_ced(struct memstick_dev *card,
drivers/memstick/core/mspro_block.c line 515
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ struct memstick_request **mrq) +{ +if ((*mrq)->error) { +complete (&card->mrq_complete); +return (*mrq)->error; + } + +dev_dbg (&card->dev,"wait for ced: value %x\n" , (*mrq)->data[0 ]); + +if ((*mrq)->data[0 ] & (MEMSTICK_INT_CMDNAK |MEMSTICK_INT_ERR )) {
elsewhere.
Quoted 11 lines
drivers/memstick/core/mspro_block.c new file +11
-0+ card->current_mrq.error = -EFAULT ; +complete (&card->mrq_complete); +return card->current_mrq.error; + } + +if (!((*mrq)->data[0 ] &MEMSTICK_INT_CED )) +return 0 ; +else { + card->current_mrq.error =0 ; +complete (&card->mrq_complete); +return -EAGAIN ;
drivers/memstick/core/mspro_block.c line 536
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ } +} + +static int h_mspro_block_transfer_data (struct memstick_dev *card, +struct memstick_request **mrq) +{ +struct memstick_host *host = card->host; +struct mspro_block_data *msb =memstick_get_drvdata (card); +unsigned char t_val =0 ; +struct scatterlist t_sg = {0 };
Should this be using sg_init_table()?
Quoted 41 lines
drivers/memstick/core/mspro_block.c new file +38
-0+size_t t_offset; + +if ((*mrq)->error) { +complete (&card->mrq_complete); +return (*mrq)->error; + } + +switch ((*mrq)->tpc) { +case MS_TPC_WRITE_REG : +memstick_init_req (*mrq,MS_TPC_SET_CMD , &msb->transfer_cmd,1 ); + (*mrq)->get_int_reg =1 ; +return 0 ; +case MS_TPC_SET_CMD : + t_val = (*mrq)->int_reg; +memstick_init_req (*mrq,MS_TPC_GET_INT ,NULL ,1 ); +if (host->caps &MEMSTICK_CAP_AUTO_GET_INT ) + goto has_int_reg; +return 0 ; + +static void mspro_block_process_request(struct memstick_dev *card, + struct request *req) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct mspro_param_register param; +int rc, chunk, cnt; +unsigned short page_count; +sector_t t_sec; +unsigned long flags; + + do { + page_count = 0; + msb->current_seg = 0; + msb->seg_count = blk_rq_map_sg(req->q, req, msb->req_sg); + + if (msb->seg_count) { + msb->current_page = 0; + for (rc = 0; rc < msb->seg_count; rc++) + page_count += msb->req_sg[rc].length
drivers/memstick/core/mspro_block.c line 650
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ / msb->page_size; + + t_sec = req->sector; +sector_div (t_sec, msb->page_size >>9 ); + param = (struct mspro_param_register ) { + .system = msb->system, + .data_count =cpu_to_be16 (page_count), + .data_address =cpu_to_be32 ((uint32_t )t_sec), + .cmd_param =0 + };
Might generate bad code.
Quoted 77 lines
drivers/memstick/core/mspro_block.c new file +73
-0+ msb->data_dir =rq_data_dir (req); + msb->transfer_cmd = msb->data_dir ==READ + ?MSPRO_CMD_READ_DATA + :MSPRO_CMD_WRITE_DATA ; + +dev_dbg (&card->dev,"data transfer: cmd %x, " +"lba %x, count %x\n" , msb->transfer_cmd, +be32_to_cpu (param.data_address), + page_count); + + card->next_request = h_mspro_block_req_init; + msb->mrq_handler = h_mspro_block_transfer_data; +memstick_init_req (&card->current_mrq,MS_TPC_WRITE_REG , + ¶m,sizeof (param)); +memstick_new_req (card->host); +wait_for_completion (&card->mrq_complete); + rc = card->current_mrq.error; + +if (rc || (card->current_mrq.tpc ==MSPRO_CMD_STOP )) { +for (cnt =0 ; cnt < msb->current_seg; cnt++) + page_count += msb->req_sg[cnt].length + / msb->page_size; + +if (msb->current_page) + page_count += msb->current_page -1 ; + +if (page_count && (msb->data_dir ==READ )) + rc = msb->page_size * page_count; +else + rc = -EIO ; + }else + rc = msb->page_size * page_count; + }else + rc = -EFAULT ; + +spin_lock_irqsave (&msb->q_lock, flags); +if (rc >=0 ) + chunk =end_that_request_chunk (req,1 , rc); +else + chunk =end_that_request_first (req, rc, + req->current_nr_sectors); + +dev_dbg (&card->dev,"end chunk %d, %d\n" , rc, chunk); +if (!chunk) { +add_disk_randomness (req->rq_disk); +blkdev_dequeue_request (req); +end_that_request_last (req, rc >0 ?1 : rc); + } +spin_unlock_irqrestore (&msb->q_lock, flags); + }while (chunk); + +} +static int mspro_block_queue_thread (void *data) +{ +struct memstick_dev *card = data; +struct memstick_host *host = card->host; +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct request *req; +unsigned long flags; + +while (1 ) { +wait_event (msb->q_wait,mspro_block_has_request (msb)); +dev_dbg (&card->dev,"thread iter\n" ); + +spin_lock_irqsave (&msb->q_lock, flags); + req =elv_next_request (msb->queue); +dev_dbg (&card->dev,"next req %p\n" , req); +if (!req) { + msb->has_request =0 ; +if (kthread_should_stop ()) { +spin_unlock_irqrestore (&msb->q_lock, flags); +break ; + }
drivers/memstick/core/mspro_block.c line 747
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ }else + msb->has_request =1 ; +spin_unlock_irqrestore (&msb->q_lock, flags); + +if (req) { +mutex_lock (&host->lock); +mspro_block_process_request (card, req); +mutex_unlock (&host->lock); + } + }
Should this have a try_to_freeze() in it?
Quoted 13 lines
drivers/memstick/core/mspro_block.c new file +13
-0+dev_dbg (&card->dev,"thread finished\n" ); +return 0 ; +} + +static void mspro_block_request (struct request_queue *q) +{ +struct memstick_dev *card = q->queuedata; +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct request *req =NULL ; + +if (!msb->q_thread) { +for (req =elv_next_request (q); req; + req =elv_next_request (q)) {
drivers/memstick/core/mspro_block.c line 770
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+while (end_that_request_chunk (req, -ENODEV , + req->current_nr_sectors + <<9 )) {} +end_that_request_last (req, -ENODEV ); + } + }else { + msb->has_request =1 ; +wake_up_all (&msb->q_wait); + } +}
Suggest that you cc Jens on this, see if he can check it all over.
Quoted 70 lines
drivers/memstick/core/mspro_block.c new file +67
-0+/*** Initialization ***/ + +static int mspro_block_wait_for_ced (struct memstick_dev *card) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); + + card->next_request = h_mspro_block_req_init; + msb->mrq_handler = h_mspro_block_wait_for_ced; +memstick_init_req (&card->current_mrq,MS_TPC_GET_INT ,NULL ,1 ); +memstick_new_req (card->host); +wait_for_completion (&card->mrq_complete); +return card->current_mrq.error; +} +static int mspro_block_read_attributes (struct memstick_dev *card) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct mspro_param_register param = { + .system = msb->system, + .data_count =cpu_to_be16 (1 ), + .data_address =0 , + .cmd_param =0 + }; +struct mspro_attribute *attr =NULL ; +unsigned char *buffer =NULL ; +int cnt, rc; +unsigned int addr; +unsigned short page_count; + + attr =kmalloc (msb->page_size,GFP_KERNEL ); +if (!attr) +return -ENOMEM ; + +sg_init_one (&msb->req_sg[0 ], attr, msb->page_size); + msb->seg_count =1 ; + msb->current_seg =0 ; + msb->current_page =0 ; + msb->data_dir =READ ; + msb->transfer_cmd =MSPRO_CMD_READ_ATRB ; + + card->next_request = h_mspro_block_req_init; + msb->mrq_handler = h_mspro_block_transfer_data; +memstick_init_req (&card->current_mrq,MS_TPC_WRITE_REG , ¶m, +sizeof (param)); +memstick_new_req (card->host); +wait_for_completion (&card->mrq_complete); +if (card->current_mrq.error) { + rc = card->current_mrq.error; + goto out_free_attr; + } + +if (be16_to_cpu (attr->signature) !=MSPRO_BLOCK_SIGNATURE ) { +printk (KERN_ERR "%s: unrecognized device signature %x\n" , + card->dev.bus_id,be16_to_cpu (attr->signature)); + rc = -ENODEV ; + goto out_free_attr; + } + +if (attr->count >MSPRO_BLOCK_MAX_ATTRIBUTES ) { +printk (KERN_WARNING "%s: way too many attribute entries\n" , + card->dev.bus_id); + msb->attr_count =MSPRO_BLOCK_MAX_ATTRIBUTES ; + }else + msb->attr_count = attr->count; + + msb->attributes =kzalloc (msb->attr_count + *sizeof (struct mspro_sys_attr ), +GFP_KERNEL );
drivers/memstick/core/mspro_block.c line 887
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+if (!msb->attributes) { + msb->attr_count =0 ; + rc = -ENOMEM ; + goto out_free_attr; + } + + buffer =kmalloc (msb->page_size,GFP_KERNEL ); +if (!buffer) { + rc = -ENOMEM ; + goto out_free_attr;
I think we leak msb->attributes if this is taken. Please double-check the unwinding here.
drivers/memstick/core/mspro_block.c line 889
Quoted 2 lines
drivers/memstick/core/mspro_block.c new file +2
-0+ } +memcpy (buffer, (char *)attr, msb->page_size);
unneeded cast?
Quoted 19 lines
drivers/memstick/core/mspro_block.c new file +19
-0+ page_count =1 ; + +for (cnt =0 ; cnt < msb->attr_count; cnt++) { + addr =be32_to_cpu (attr->entries[cnt].address); + rc =be32_to_cpu (attr->entries[cnt].size); +dev_dbg (&card->dev,"adding attribute %d: id %x, address %x, " +"size %x\n" , cnt, attr->entries[cnt].id, addr, rc); + msb->attributes[cnt].id = attr->entries[cnt].id; +if (mspro_block_attr_name (attr->entries[cnt].id)) +snprintf (msb->attributes[cnt].name, +sizeof (msb->attributes[cnt].name),"%s" , +mspro_block_attr_name (attr->entries[cnt].id)); +else +snprintf (msb->attributes[cnt].name, +sizeof (msb->attributes[cnt].name), +"attr_x%02x" , + attr->entries[cnt].id); + + msb->attributes[cnt].sys_attr
drivers/memstick/core/mspro_block.c line 918
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+ = (struct device_attribute){ + .attr = { + .name = msb->attributes[cnt].name, + .mode =S_IRUGO , + .owner =THIS_MODULE + }, + .show =mspro_block_attr_show ( + msb->attributes[cnt].id), + .store =NULL + };
Wow, that's giving the compiler a workout. Trusting soul ;)
Alas, you may fnd the result isn't good.
Quoted 80 lines
drivers/memstick/core/mspro_block.c new file +77
-0+if (!rc) +continue ; + + msb->attributes[cnt].size = rc; + msb->attributes[cnt].data =kmalloc (rc,GFP_KERNEL ); +if (!msb->attributes[cnt].data) { + rc = -ENOMEM ; + goto out_free_buffer; + } + +if (((addr / msb->page_size) + ==be32_to_cpu (param.data_address)) + && (((addr + rc -1 ) / msb->page_size) + ==be32_to_cpu (param.data_address))) { +memcpy (msb->attributes[cnt].data, + buffer + addr % msb->page_size, + rc); +continue ; + } + +if (page_count <= (rc / msb->page_size)) { +kfree (buffer); + page_count = (rc / msb->page_size) +1 ; + buffer =kmalloc (page_count * msb->page_size, +GFP_KERNEL ); +if (!buffer) { + rc = -ENOMEM ; + goto out_free_attr; + } + } + + param = (struct mspro_param_register ){ + .system = msb->system, + .data_count =cpu_to_be16 ((rc / msb->page_size) +1 ), + .data_address =cpu_to_be32 (addr / msb->page_size), + .cmd_param =0 + }; + +sg_init_one (&msb->req_sg[0 ], buffer, +be16_to_cpu (param.data_count) * msb->page_size); + msb->seg_count =1 ; + msb->current_seg =0 ; + msb->current_page =0 ; + msb->data_dir =READ ; + msb->transfer_cmd =MSPRO_CMD_READ_ATRB ; + +dev_dbg (&card->dev,"reading attribute pages %x, %x\n" , +be32_to_cpu (param.data_address), +be16_to_cpu (param.data_count)); + + card->next_request = h_mspro_block_req_init; + msb->mrq_handler = h_mspro_block_transfer_data; +memstick_init_req (&card->current_mrq,MS_TPC_WRITE_REG , + (char *)¶m,sizeof (param)); +memstick_new_req (card->host); +wait_for_completion (&card->mrq_complete); +if (card->current_mrq.error) { + rc = card->current_mrq.error; + goto out_free_buffer; + } + +memcpy (msb->attributes[cnt].data, + buffer + addr % msb->page_size, + rc); + } + + rc =0 ; +out_free_buffer: +kfree (buffer); +out_free_attr: +kfree (attr); +return rc; +} +static int mspro_block_resume (struct memstick_dev *card) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); +unsigned long flags;
drivers/memstick/core/mspro_block.c
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+int rc =0 ; + +#ifdef CONFIG_MEMSTICK_UNSAFE_RESUME + +struct mspro_block_data *new_msb; +struct memstick_host *host = card->host; +unsigned char cnt; + +mutex_lock (&host->lock); + new_msb =kzalloc (sizeof (struct mspro_block_data ),GFP_KERNEL );
If anything takes host->lock on the IO path then there might be a deadlock here. GFP_NOIO, perhaps. or just swap these two lines.
drivers/memstick/core/mspro_block.c line 1278
Quoted 8 lines
drivers/memstick/core/mspro_block.c new file +8
-0+if (!new_msb) { + rc = -ENOMEM ; + goto out_unlock; + } + + new_msb->card = card; +memstick_set_drvdata (card, new_msb); +if (mspro_block_init_card (card))
except this might do allocations too, dunno.
Quoted 27 lines
drivers/memstick/core/mspro_block.c new file +27
-0+ goto out_free; + +for (cnt =0 ; cnt < new_msb->attr_count; cnt++) { +if (new_msb->attributes[cnt].id ==MSPRO_BLOCK_ID_SYSINFO + && cnt < msb->attr_count + && msb->attributes[cnt].id ==MSPRO_BLOCK_ID_SYSINFO ) { +if (memcmp (new_msb->attributes[cnt].data, + msb->attributes[cnt].data, + msb->attributes[cnt].size)) +break ; + +memstick_set_drvdata (card, msb); + msb->q_thread =kthread_run (mspro_block_queue_thread, + card,DRIVER_NAME "d" ); +if (IS_ERR (msb->q_thread)) + msb->q_thread =NULL ; +else + msb->active =1 ; + +break ; + } + } + +out_free: +memstick_set_drvdata (card, msb); +mspro_block_data_clear (new_msb); +kfree (new_msb);
drivers/memstick/core/mspro_block.c line 1315
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+out_unlock: +mutex_unlock (&host->lock); + +#endif /* CONFIG_MEMSTICK_UNSAFE_RESUME */ + +spin_lock_irqsave (&msb->q_lock, flags); +blk_start_queue (msb->queue); +spin_unlock_irqrestore (&msb->q_lock, flags); +return rc; +}
Here my attention span ran out. Except for...
Quoted 4 lines
include/linux/memstick.h new file +4
-0+inline void *memstick_priv (struct memstick_host *host) +{ +return (void *)host->private; +}
include/linux/memstick.h line 287
Quoted 10 lines
include/linux/memstick.h new file +10
-0+ +inline void *memstick_get_drvdata (struct memstick_dev *card) +{ +return dev_get_drvdata (&card->dev); +} + +inline void memstick_set_drvdata (struct memstick_dev *card,void *data) +{ +dev_set_drvdata (&card->dev, data); +}
static inline.
Generally: a clean and idiomatic-looking driver but I am not able to review it very effectively due to its extraordinary lack of commentary.
Quoted 13 lines · Andrew Morton
Quoted 13 lines
drivers/memstick/core/mspro_block.c new file +13
-0+static void mspro_block_request (struct request_queue *q) +{ +struct memstick_dev *card = q->queuedata; +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct request *req =NULL ; + +if (!msb->q_thread) { +for (req =elv_next_request (q); req; + req =elv_next_request (q)) { +while (end_that_request_chunk (req, -ENODEV , + req->current_nr_sectors + <<9 )) {} +end_that_request_last (req, -ENODEV );
Quoted 8 lines
drivers/memstick/core/mspro_block.c line 770
Quoted 6 lines
drivers/memstick/core/mspro_block.c new file +6
-0+ } + }else { + msb->has_request =1 ; +wake_up_all (&msb->q_wait); + } +}Suggest that you cc Jens on this, see if he can check it all over.
It's suboptimal and doesn't work for non-fs request. Just use end_queued_request() instead:
if (msb->q_thread) {
msb->has_request = 1;
wake_up(&msb->q_wait);
} else {
while ((req = elv_next_request(q)) != NULL)
end_queued_request(req, -ENODEV);
}which is simpler and gets all cases correct. Reordering for normal case as well.
Signature · 1 line
Jens Axboe
Alex Dubov
maintainerQuoted 10 lines · --- Andrew Morton
Quoted 7 lines · On Wed, 2 Jan 2008 17:42:24 +1100 [email protected] wrote:
From: Alex Dubov <[email protected]>
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
Quoted 7 lines
Will you be running a git tree for this? If so, please send me the link.
Will this enable that thus-far useless slot on my Vaio?
Where did the info come from which enabled this driver to be written? I thought Sony were super-secretive about this stuff?
I'm going to set-up git tree, yes.
It should support some vaios (newer ones for sure), as far as I know.
The primary reference for the driver is this one:
http://zeniv.linux.org.uk/~winbond/ (link is somewhat slow, but works). Winbond developed GPLv2 driver for linux and there was enough info in their source code for my implementation (totally different from their's). Some reverse engineering of the windows driver was needed too (for the TI registers and such).
Quoted 15 lines
drivers/memstick/Kconfig line 11
Quoted 13 lines
drivers/memstick/Kconfig new file +11
-0@@ -0,0 +1,26 @@ +# +# MemoryStick subsystem configuration +# + +menuconfig MEMSTICK + tristate "Sony MemoryStick card support (EXPERIMENTAL)" + help + Sony MemoryStick is a proprietary storage/extension card protocol. + + If you want MemoryStick support, you should say Y here and also + to the specific driver for your MMC interface.Are you sure this has enough dependencies? CONFIG_TIFM_* for a start?
This is supposed to be a generic layer, akin to mmc. I have a jmicron backend in the works, and windbond backend will be possible, too.
Quoted 7 lines
<fiddles with
Kconfig>We can create a config which has
CONFIG_TIFM_CORE=m,CONFIG_MEMSTICK=y. That might not work?Anyway, please have a think about that.
Considering the previous point this should be ok.
Quoted 21 lines
Quoted 21 lines
...
drivers/memstick/core/mspro_block.c new file +18
-0+ +struct mspro_sys_attr { +size_t size; +unsigned char *data; +unsigned char id; +char name[32 ]; +struct device_attribute sys_attr; +}; + +struct mspro_attr_entry { +unsigned int address; +unsigned int size; +unsigned char id; +unsigned char reserved[3 ]; +} __attribute__((packed)); + +struct mspro_attribute { +unsigned short signature;
Quoted 8 lines
drivers/memstick/core/mspro_block.c line 66
Quoted 5 lines
drivers/memstick/core/mspro_block.c new file +5
-0+unsigned short version; +unsigned char count; +unsigned char reserved[11 ]; +struct mspro_attr_entry entries[]; +} __attribute__((packed));Why are these packed? Do they map onto something whcih hardware knows about?
Yes, of course; all structures with "packed" attribute correspond to appropriate protocol ones (the endianness is tweaked as needed - most, but not all fields, are big-endian).
Quoted 28 lines
Quoted 12 lines
...
drivers/memstick/core/mspro_block.c new file +9
-0+struct mspro_block_data { +struct memstick_dev *card; +unsigned int usage_count; +struct gendisk *disk; +struct request_queue *queue; +spinlock_t q_lock; +wait_queue_head_t q_wait; +struct task_struct *q_thread; +drivers/memstick/core/mspro_block.c line 141
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+unsigned short page_size; +unsigned short cylinders; +unsigned short heads; +unsigned short sectors_per_track; + +unsigned char system; + unsigned char read_only:1, + active:1, + has_request:1, + data_dir:1;You're aware that these four fields occupy the same machine word and that the compiler provides no locking? So if one thread attempts to modify "
read_only" while another modifies "has_request", one can corrupt the work of the other?
Quoted 2 lines
If this is indeed safe then a comment here whcih clearly explains that would be useful.
The access to these fields should be exclusively under q_lock (I'll check the locking again, just to make sure). After all, the same applies to old fashioned bit masks.
Quoted 35 lines
Quoted 18 lines
drivers/memstick/core/mspro_block.c new file +14
-0+} + +static ssize_t mspro_block_attr_show_modelname (struct device *dev, +struct device_attribute *attr, +char *buffer) +{ +struct mspro_sys_attr *x_attr =container_of (attr, + struct mspro_sys_attr, + sys_attr); + +return snprintf (buffer,PAGE_SIZE ,"%s" , x_attr->data); +} +static int mspro_block_sysfs_register (struct memstick_dev *card) +{drivers/memstick/core/mspro_block.c line 443
Quoted 10 lines
drivers/memstick/core/mspro_block.c new file +10
-0+struct mspro_block_data *msb =memstick_get_drvdata (card); +int cnt, rc =0 ; + +for (cnt =0 ; cnt < msb->attr_count; cnt++) { + rc =device_create_file (&card->dev, + &msb->attributes[cnt].sys_attr); + +if (rc) { +if (cnt) { +for (cnt--; cnt >=0 ; cnt--)ow. my brain hurts.
The
if (cnt)' is actually unneeded. But if it were mine I'd be doing something simpler and obviouser here. Likefor (i = 0; i < cnt; i++)' simple == good. !simple == !.
Quoted 11 lines
drivers/memstick/core/mspro_block.c line 452
Quoted 9 lines
drivers/memstick/core/mspro_block.c new file +9
-0+device_remove_file (&card->dev, + &msb->attributes[cnt] + .sys_attr); + } +break ; + } + } +return rc; +}Could we be using attribute groups for all this?
The idea here is that MSPro media have an attribute namespace, which may contain various entries. Some of them I know how to decode, others I still want to be presented in sysfs for further study (as hex dumps). Sysfs attr group should be fine, but I was not aware that such thing exists.
Quoted 28 lines
Quoted 28 lines
Quoted 1 line
...
drivers/memstick/core/mspro_block.c new file +25
-0+static int mspro_block_queue_thread (void *data) +{ +struct memstick_dev *card = data; +struct memstick_host *host = card->host; +struct mspro_block_data *msb =memstick_get_drvdata (card); +struct request *req; +unsigned long flags; + +while (1 ) { +wait_event (msb->q_wait,mspro_block_has_request (msb)); +dev_dbg (&card->dev,"thread iter\n" ); + +spin_lock_irqsave (&msb->q_lock, flags); + req =elv_next_request (msb->queue); +dev_dbg (&card->dev,"next req %p\n" , req); +if (!req) { + msb->has_request =0 ; +if (kthread_should_stop ()) { +spin_unlock_irqrestore (&msb->q_lock, flags); +break ; + } + }else + msb->has_request =1 ; +spin_unlock_irqrestore (&msb->q_lock, flags); +
Quoted 8 lines
drivers/memstick/core/mspro_block.c line 747
Quoted 6 lines
drivers/memstick/core/mspro_block.c new file +6
-0+if (req) { +mutex_lock (&host->lock); +mspro_block_process_request (card, req); +mutex_unlock (&host->lock); + } + }Should this have a
try_to_freeze()in it?
The thread is stopped on suspend and restarted on resume.It works on my machine.
Quoted 11 lines
Quoted 11 lines
...
drivers/memstick/core/mspro_block.c new file +9
-0+static int mspro_block_resume (struct memstick_dev *card) +{ +struct mspro_block_data *msb =memstick_get_drvdata (card); +unsigned long flags; +int rc =0 ; + +#ifdef CONFIG_MEMSTICK_UNSAFE_RESUME + +struct mspro_block_data *new_msb;
Quoted 8 lines
drivers/memstick/core/mspro_block.c line 1270
Quoted 5 lines
drivers/memstick/core/mspro_block.c new file +5
-0+struct memstick_host *host = card->host; +unsigned char cnt; + +mutex_lock (&host->lock); + new_msb =kzalloc (sizeof (struct mspro_block_data ),GFP_KERNEL );If anything takes
host->lockon the IO path then there might be a deadlock here.GFP_NOIO, perhaps. or just swap these two lines.
There's no IO path at this point, as IO thread was stopped on suspend. The fine point here is that thread is restarted only with "UNSAFE_RESUME" set, otherwise the block device just sits dead in the water waiting for the bus driver to kick it out and run the media detection routine anew.
Quoted 8 lines
Quoted 8 lines
include/linux/memstick.h new file +8
-0+inline void *memstick_priv (struct memstick_host *host) +{ +return (void *)host->private; +} + +inline void *memstick_get_drvdata (struct memstick_dev *card) +{ +return dev_get_drvdata (&card->dev);
Quoted 9 lines
include/linux/memstick.h line 287
Quoted 6 lines
include/linux/memstick.h new file +6
-0+} + +inline void memstick_set_drvdata (struct memstick_dev *card,void *data) +{ +dev_set_drvdata (&card->dev, data); +}static inline.
Somebody was already kind enough to fix this.
I hope it'll be ok for me to address the rest of the issues with incremental patches.I expect to have a web accessible git within a few days.
____________________________________________________________________________________
Looking for last minute shopping deals?
Find them fast with Yahoo! Search. http://tools.search.yahoo.com/newsearch/category.php?category=shopping
Alex Dubov
maintainerQuoted 4 lines · --- Andrew Morton
Quoted 2 lines · On Wed, 2 Jan 2008 17:42:24 +1100 [email protected] wrote:
From: Alex Dubov <[email protected]>
Quoted 8 lines
Quoted 5 lines
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
Will you be running a git tree for this? If so, please send me the link.
I rectified several additional issues with the driver. I hope you can pull the changes from here:
http://58.96.64.63/~oakad/git/.git memstick
____________________________________________________________________________________
Be a better friend, newshound, and
know-it-all with Yahoo! Mobile. Try it now. http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
Quoted 14 lines · On Tue, 22 Jan 2008 08:12:08 -0800 (PST) Alex Dubov <[email protected]> wrote:
Quoted 10 lines · --- Andrew Morton
Quoted 7 lines · On Wed, 2 Jan 2008 17:42:24 +1100 [email protected] wrote:
From: Alex Dubov <[email protected]>
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
Quoted 7 lines
Quoted 2 lines
Will you be running a git tree for this? If so, please send me the link.
I rectified several additional issues with the driver. I hope you can pull the changes from here:
http://58.96.64.63/~oakad/git/.git memstick
Would prefer an emailed, changelogged diff against the previous version, please.
Alex Dubov
maintainerPATCH 1/1 Updates for the memstick driver
* Mark shared inline functions as static
* Use member-at-a-time assignment for protocol structures
* Comments for publicly exported functions
* Use end_queued_request to end unhandled block layer requests
* Use sysfs attribute group to export MSPro attributes
* Fix includes
* Use scnprintf instead of snprintf where string length matters
* Remove spurious get_device/put_device in probe method
drivers/memstick/core/memstick.c +16 -5
@@ -12,11 +12,10 @@* */ -#include <linux/tifm.h> #include <linux/memstick.h> #include <linux/idr.h> -#include <linux/scatterlist.h> #include <linux/fs.h> +#include <linux/delay.h> #define DRIVER_NAME "memstick"#define DRIVER_VERSION "0.2"
@@ -86,13 +85,11 @@ static int memstick_device_probe(struct device *dev) driver);int rc = -ENODEV ; -get_device (dev);if (dev->driver && drv->probe) { rc = drv->probe (card);if (!rc) -return 0 ; +get_device (dev); } -put_device (dev);return rc; }
@@ -205,12 +202,26 @@ static int memstick_dummy_check(struct memstick_dev *card)return 0 ; } +/** +* memstick_detect_change - schedule media detection on memstick host +* @host - host to use +*/ void memstick_detect_change (struct memstick_host *host) {queue_work (workqueue, &host->media_checker); }EXPORT_SYMBOL (memstick_detect_change); +/** +* memstick_next_req - called by host driver to obtain next request to process +* @host - host to use +* @mrq - pointer to stick the request to +* +* Host calls this function from idle state (*mrq == NULL) or after finishing +* previous request (*mrq should point to it). If previous request was +* unsuccessful, it is retried for predetermined number of times. Return value +* of 0 means that new request was assigned to the host. +*/ int memstick_next_req (struct memstick_host *host,struct memstick_request **mrq) {int rc = -ENXIO ;
@@ -233,6 +244,10 @@ int memstick_next_req(struct memstick_host *host, struct memstick_request
**mrq)
}
EXPORT_SYMBOL(memstick_next_req);
+/**
+ * memstick_new_req - notify the host that some requests are pending
+ * @host - host to use
+ */
void memstick_new_req(struct memstick_host *host)
{
host->retries = cmd_retries;
@@ -240,6 +255,12 @@ void memstick_new_req(struct memstick_host *host)
}
EXPORT_SYMBOL(memstick_new_req);
+/**
+ * memstick_init_req_sg - set request fields needed for bulk data transfer
+ * @mrq - request to use
+ * @tpc - memstick Transport Protocol Command
+ * @sg - TPC argument
+ */
void memstick_init_req_sg(struct memstick_request *mrq, unsigned char tpc,
struct scatterlist *sg)
{
@@ -261,6 +282,17 @@ void memstick_init_req_sg(struct memstick_request *mrq, unsigned char tpc,
}
EXPORT_SYMBOL(memstick_init_req_sg);
+/**
+ * memstick_init_req - set request fields needed for short data transfer
+ * @mrq - request to use
+ * @tpc - memstick Transport Protocol Command
+ * @buf - TPC argument buffer
+ * @length - TPC argument size
+ *
+ * The intended use of this function (transfer of data items several bytes
+ * in size) allows us to just copy the value between request structure and
+ * user supplied buffer.
+ */
void memstick_init_req(struct memstick_request *mrq, unsigned char tpc,
void *buf, size_t length)
{
@@ -285,6 +317,13 @@ void memstick_init_req(struct memstick_request *mrq, unsigned char tpc,
}
EXPORT_SYMBOL(memstick_init_req);
+/*
+ * Functions prefixed with "h_" are protocol callbacks. They can be called from
+ * interrupt context. Return value of 0 means that request processing is still
+ * ongoing, while special error value of -EAGAIN means that current request is
+ * finished (and request processor should come back some time later).
+ */
+
static int h_memstick_read_dev_id(struct memstick_dev *card,
struct memstick_request **mrq)
{
@@ -298,12 +337,10 @@ static int h_memstick_read_dev_id(struct memstick_dev *card,
} else {
if (!(*mrq)->error) {
memcpy(&id_reg, (*mrq)->data, sizeof(id_reg));
- card->id = (struct memstick_device_id){
- .match_flags = MEMSTICK_MATCH_ALL,
- .type = id_reg.type,
- .category = id_reg.category,
- .class = id_reg.class
- };
+ card->id.match_flags = MEMSTICK_MATCH_ALL;
+ card->id.type = id_reg.type;
+ card->id.category = id_reg.category;
+ card->id.class = id_reg.class;
}
complete(&card->mrq_complete);
return -EAGAIN;
@@ -325,6 +362,11 @@ static int h_memstick_set_rw_addr(struct memstick_dev *card,
}
}
+/**
+ * memstick_set_rw_addr - issue SET_RW_REG_ADDR request and wait for it to
+ * complete
+ * @card - media device to use
+ */
int memstick_set_rw_addr(struct memstick_dev *card)
{
card->next_request = h_memstick_set_rw_addr;
@@ -351,12 +393,10 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
card->dev.release = memstick_free_card;
card->check = memstick_dummy_check;
- card->reg_addr = (struct ms_register_addr){
- offsetof(struct ms_register, id),
- sizeof(id_reg),
- offsetof(struct ms_register, id),
- sizeof(id_reg)
- };
+ card->reg_addr.r_offset = offsetof(struct ms_register, id);
+ card->reg_addr.r_length = sizeof(id_reg);
+ card->reg_addr.w_offset = offsetof(struct ms_register, id);
+ card->reg_addr.w_length = sizeof(id_reg);
init_completion(&card->mrq_complete);
@@ -433,6 +473,11 @@ static void memstick_check(struct work_struct *work)
dev_dbg(host->cdev.dev, "memstick_check finished\n");
}
+/**
+ * memstick_alloc_host - allocate a memstick_host structure
+ * @extra: size of the user private data to allocate
+ * @dev: parent device of the host
+ */
struct memstick_host *memstick_alloc_host(unsigned int extra,
struct device *dev)
{
@@ -450,6 +495,10 @@ struct memstick_host *memstick_alloc_host(unsigned int extra,
}
EXPORT_SYMBOL(memstick_alloc_host);
+/**
+ * memstick_add_host - start request processing on memstick host
+ * @host - host to use
+ */
int memstick_add_host(struct memstick_host *host)
{
int rc;
@@ -480,6 +529,10 @@ int memstick_add_host(struct memstick_host *host)
}
EXPORT_SYMBOL(memstick_add_host);
+/**
+ * memstick_remove_host - stop request processing on memstick host
+ * @host - host to use
+ */
void memstick_remove_host(struct memstick_host *host)
{
flush_workqueue(workqueue);
@@ -497,6 +550,10 @@ void memstick_remove_host(struct memstick_host *host)
}
EXPORT_SYMBOL(memstick_remove_host);
+/**
+ * memstick_free_host - free memstick host
+ * @host - host to use
+ */
void memstick_free_host(struct memstick_host *host) {
mutex_destroy(&host->lock);
drivers/memstick/core/mspro_block.c +205 -219
632 lines not on this page · raw message
include/linux/memstick.h +13 -3
@@ -13,6 +13,10 @@#define _MEMSTICK_H#include <linux/workqueue.h> +#include <linux/scatterlist.h> +#include <linux/device.h> + +/*** Hardware based structures ***/ struct ms_status_register {unsigned char reserved;
@@ -151,6 +155,8 @@ enum {*/ }; +/*** Driver structures and functions ***/ +#define MEMSTICK_PART_SHIFT 3enum memstick_param {MEMSTICK_POWER =1 ,MEMSTICK_INTERFACE };
@@ -212,7 +218,9 @@ struct memstick_dev {struct completion mrq_complete;struct memstick_request current_mrq; +/* Check that media driver is still willing to operate the device. */ int (*check)(struct memstick_dev *card); +/* Get next request from the media driver. */ int (*next_request)(struct memstick_dev *card,struct memstick_request **mrq);
@@ -232,7 +240,9 @@ struct memstick_host {struct memstick_dev *card;unsigned int retries; +/* Notify the host that some requests are pending. */ void (*request)(struct memstick_host *host); +/* Set host IO parameters (power, clock, etc). */ void (*set_param)(struct memstick_host *host,enum memstick_param param,int value);
@@ -271,17 +281,17 @@ void memstick_new_req(struct memstick_host *host);int memstick_set_rw_addr (struct memstick_dev *card); -inline void *memstick_priv (struct memstick_host *host) +static inline void *memstick_priv (struct memstick_host *host) {return (void *)host->private; } -inline void *memstick_get_drvdata (struct memstick_dev *card) +static inline void *memstick_get_drvdata (struct memstick_dev *card) {return dev_get_drvdata (&card->dev); } -inline void memstick_set_drvdata (struct memstick_dev *card,void *data) +static inline void memstick_set_drvdata (struct memstick_dev *card,void *data) {dev_set_drvdata (&card->dev, data); }
____________________________________________________________________________________
Be a better friend, newshound, and
know-it-all with Yahoo! Mobile. Try it now. http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
Re: [PATCH] [MEMSTICK] Updates for the memstick driver
Quoted 9 lines · On Thu, 24 Jan 2008 23:58:52 -0800 (PST) Alex Dubov <[email protected]> wrote:
* Mark shared inline functions as static
* Use member-at-a-time assignment for protocol structures
* Comments for publicly exported functions
* Use
end_queued_requestto end unhandled block layer requests
Quoted 8 lines
* Use sysfs attribute group to export MSPro attributes
* Fix includes
* Use scnprintf instead of snprintf where string length matters
* Remove spurious get_device/put_device in probe method
Please sign off your patches.
Please don't send wordwrapped patches.
Please put the subsystem identifier "memstick" outside [], for reasons described in http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
Your patch contained a fix which I already merged into -mm, which suggests that you haven't been testing the code in 2.6.24-rc8-mm1. Please do test -mm when you have patches in there, so that we know that we'll be merging things which work.
Thanks.
Re: [PATCH] [MEMSTICK] Updates for the memstick driver
In reply to Alex DubovChanges to the block core in mainline have destroyed this driver. This was hitherto not known because I was unable to carry git-block in -mm because it blithely tromped all over other people's code.
I'll disable the memstick driver in config for now. Please send fixes?
Alex Dubov
maintainerPATCH 1/1 memstick: use __blk_end_request to complete requests
mspro_block.c renamed from mspro_block.c.orig +2 -9
@@ -668,20 +668,13 @@spin_lock_irqsave (&msb->q_lock, flags);if (rc >=0 ) - chunk =end_that_request_chunk (req,1 , rc); + chunk =__blk_end_request (req,0 , rc);else - chunk =end_that_request_first (req, rc, - req->current_nr_sectors); + chunk =__blk_end_request (req, rc,0 );dev_dbg (&card->dev,"end chunk %d, %d\n" , rc, chunk); -if (!chunk) { -add_disk_randomness (req->rq_disk); -blkdev_dequeue_request (req); -end_that_request_last (req, rc >0 ?1 : rc); - }spin_unlock_irqrestore (&msb->q_lock, flags); }while (chunk); - }static int mspro_block_has_request (struct mspro_block_data *msb)
____________________________________________________________________________________
Be a better friend, newshound, and
know-it-all with Yahoo! Mobile. Try it now. http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
Re: [PATCH] memstick: use __blk_end_request to complete requests
Quoted 18 lines · On Sun, 3 Feb 2008 20:31:10 -0800 (PST) Alex Dubov <[email protected]> wrote:
Signed-off-by Alex Dubov <[email protected]>mspro_block.c renamed from mspro_block.c.orig +2
-4@@ -668,20 +668,13 @@spin_lock_irqsave (&msb->q_lock, flags);if (rc >=0 ) - chunk =end_that_request_chunk (req,1 , rc); + chunk =__blk_end_request (req,0 , rc);else - chunk =end_that_request_first (req, rc, - req->current_nr_sectors); + chunk =__blk_end_request (req, rc,0 );dev_dbg (&card->dev,"end chunk %d, %d\n" , rc, chunk); -if (!chunk) {
mspro_block.c line 680
Quoted 10 lines
mspro_block.c renamed from mspro_block.c.orig +0
-5-add_disk_randomness (req->rq_disk); -blkdev_dequeue_request (req); -end_that_request_last (req, rc >0 ?1 : rc); - }spin_unlock_irqrestore (&msb->q_lock, flags); }while (chunk); - }static int mspro_block_has_request (struct mspro_block_data *msb)
Thanks. However please do prepare patches in `patch -p1' form.
Could you please fix the build error in the code in 2.6.24-mm1?
I part-fixed it (then disabled it) with this:
drivers/memstick/core/mspro_block.c renamed from drivers/memstick/core/mspro_block.c~a +3 -2
@@ -1233,11 +1232,12 @@ static int mspro_block_resume(struct memunsigned long flags;int rc =0 ; -#ifdef CONFIG_MEMSTICK_UNSAFE_RESUME +#if defined(CONFIG_MEMSTICK_UNSAFE_RESUME ) &&0 struct mspro_block_data *new_msb;struct memstick_host *host = card->host; -struct mspro_sys_attr s_attr, r_attr; +struct mspro_sys_attr *s_attr; +struct mspro_sys_attr *r_attr;unsigned char cnt;mutex_lock (&host->lock);
_
see, this:
s_attr = container_of(new_msb->attr_group.attrs[cnt], struct mspro_sys_attr, dev_attr);
is broken. Attribute groups hold struct attribute' but this code thinks they hold struct device_attribute'. I could bodge it to compile cleanly, but I don't know if it will work.
Alex Dubov
maintainerPATCH 1/1 memstick: fix attribute structure casting in mspro_block_resume
--- drivers/memstick/core/mspro_block.c | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-)
drivers/memstick/core/mspro_block.c +3 -7
@@ -1237,7 +1237,7 @@ static int mspro_block_resume(struct memstick_dev *card)struct mspro_block_data *new_msb;struct memstick_host *host = card->host; -struct mspro_sys_attr s_attr, r_attr; +struct mspro_sys_attr *s_attr, *r_attr;unsigned char cnt;mutex_lock (&host->lock);
@@ -1254,12 +1254,8 @@ static int mspro_block_resume(struct memstick_dev *card)for (cnt =0 ; new_msb->attr_group.attrs[cnt] && msb->attr_group.attrs[cnt]; ++cnt) { - s_attr =container_of (new_msb->attr_group.attrs[cnt], - struct mspro_sys_attr, - dev_attr); - r_attr =container_of (msb->attr_group.attrs[cnt], - struct mspro_sys_attr, - dev_attr); + s_attr =mspro_from_sysfs_attr (new_msb->attr_group.attrs[cnt]); + r_attr =mspro_from_sysfs_attr (msb->attr_group.attrs[cnt]);if (s_attr->id ==MSPRO_BLOCK_ID_SYSINFO && r_attr->id == s_attr->id) {
Signature · 6 lines
1.5.3.6
____________________________________________________________________________________
Looking for last minute shopping deals?
Find them fast with Yahoo! Search. http://tools.search.yahoo.com/newsearch/category.php?category=shoppingHello,
Quoted 4 lines
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
I tried it here and it doesn't work. My Vaio (PCG-FR285M) is from ~2003 (Is it too old for this?). I have some memory stick cards around so If you want a tester just drop me an email.
Regards,
Mariusz
Alex Dubov
maintainerQuoted 7 lines · --- Mariusz Kozlowski
Hello,
Quoted 4 lines
Sony MemoryStick cards are used in many products manufactured by Sony. They are available both as storage and as IO expansion cards. Currently, only MemoryStick Pro storage cards are supported via TI FlashMedia MemoryStick interface.
Quoted 8 lines
I tried it here and it doesn't work. My Vaio (PCG-FR285M) is from ~2003 (Is it too old for this?). I have some memory stick cards around so If you want a tester just drop me an email.
Regards,
Mariusz
The build year is nowhere as helpful as 'lspci -vv' output. Then, given that your vaio is equipped with tifm controller, you'll have to build the driver with debugging enabled and send me the relevant excerpt of your system log.
You should have the following modules loaded, by the way:
memstickmspro_blocktifm_coretifm_7xx1tifm_ms
The autoloading is handled via udev (so the relevant rules are not there yet).
____________________________________________________________________________________
Never miss a thing. Make Yahoo your home page.
http://www.yahoo.com/r/hs