Re: [PATCH 1/3] mfd: Add realtek USB card reader driver

From: Dan Carpenter
Date: Thu Jan 02 2014 - 04:48:18 EST

Next message: Jiri Olsa: "[PATCHv2 1/2] tools lib traceevent: Replace tabs with spaces for allnon-commands statements"
Previous message: Jonathan Cameron: "Re: [PATCHv5 RESEND 4/4] iio: mxs-lradc: convert is_divided to a bitmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Dec 23, 2013 at 05:52:05PM +0800, rogerable@xxxxxxxxxxx wrote:
> +static int rtsx_usb_seq_write_register(struct rtsx_ucr *ucr,
> + u16 addr, u16 len, u8 *data)
> +{
> + u16 cmd_len = len + 12;
> +
> + if (data == NULL)
> + return -EINVAL;
> +
> + cmd_len = (cmd_len <= CMD_BUF_LEN) ? cmd_len : CMD_BUF_LEN;
> +

I do not like how you have three names for the same buffer length.

> +#define IOBUF_SIZE 1024
> +#define CMD_BUF_LEN 1024
> +#define RSP_BUF_LEN 1024

The buffer is allocated as IOBUF_SIZE and then CMD_BUF_LEN is used as
a limiter here and one other place. RSP_BUF_LEN is not used at all.

> + if (cmd_len % 4)
> + cmd_len += (4 - cmd_len % 4);
> +
> +
> + ucr->cmd_buf[0] = 'R';
> + ucr->cmd_buf[1] = 'T';
> + ucr->cmd_buf[2] = 'C';
> + ucr->cmd_buf[3] = 'R';
> + ucr->cmd_buf[PACKET_TYPE] = SEQ_WRITE;
> + ucr->cmd_buf[5] = (u8)(len >> 8);
> + ucr->cmd_buf[6] = (u8)len;
> + ucr->cmd_buf[STAGE_FLAG] = 0;
> + ucr->cmd_buf[8] = (u8)(addr >> 8);
> + ucr->cmd_buf[9] = (u8)addr;
> +
> + memcpy(ucr->cmd_buf + 12, data, len);

Buffer overflow here because ->cmd_buf is only IOBUF_SIZE long.
Probably the earlier two uses of "len" are buffer overflows as well.

> +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> + u8 mask, u8 data)
> +{
> + u16 value = 0, index = 0;
> +
> + value |= (u16)(3 & 0x03) << 14;
> + value |= (u16)(addr & 0x3FFF);

Don't do pointless things:

value |= 0x03 << 14;
value |= addr & 0x3FFF;

> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);

This is an endian conversion? It is buggy. Use the kernel endian
conversion functions cpu_to_le16().

> + index |= (u16)mask;
> + index |= (u16)data << 8;
> +
> + return usb_control_msg(ucr->pusb_dev,
> + usb_sndctrlpipe(ucr->pusb_dev, 0), 0, 0x40,
> + value, index, NULL, 0, 100);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_write_register);
> +
> +int rtsx_usb_ep0_read_register(struct rtsx_ucr *ucr, u16 addr, u8 *data)
> +{
> + u16 value = 0;
> +
> + if (data == NULL)
> + return -EINVAL;
> + *data = 0;
> +
> + value |= (u16)(2 & 0x03) << 14;
> + value |= (u16)(addr & 0x3FFF);
> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);

same.

The rest of my comments below are minor white space nits.

> +
> + return usb_control_msg(ucr->pusb_dev,
> + usb_rcvctrlpipe(ucr->pusb_dev, 0), 0, 0xC0,
> + value, 0, data, 1, 100);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_read_register);
> +
> +void rtsx_usb_add_cmd(struct rtsx_ucr *ucr,
> + u8 cmd_type,
> + u16 reg_addr,
> + u8 mask,
> + u8 data)
> +{
> + int i;
> +
> + if (ucr->cmd_idx < ((CMD_BUF_LEN - CMD_OFFSET) / 4)) {
> + i = CMD_OFFSET + ucr->cmd_idx * 4;
> +
> + ucr->cmd_buf[i++] = ((cmd_type & 0x03) << 6) |
> + (u8)((reg_addr >> 8) & 0x3F);
> + ucr->cmd_buf[i++] = (u8)reg_addr;
> + ucr->cmd_buf[i++] = mask;
> + ucr->cmd_buf[i++] = data;
> +
> + ucr->cmd_idx++;
> + }
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_add_cmd);
> +
> +int rtsx_usb_send_cmd(struct rtsx_ucr *ucr, u8 flag, int timeout)
> +{
> + int ret;
> +
> + ucr->cmd_buf[CNT_H] = (u8)(ucr->cmd_idx >> 8);
> + ucr->cmd_buf[CNT_L] = (u8)(ucr->cmd_idx);
> + ucr->cmd_buf[STAGE_FLAG] = flag;
> +
> + ret = rtsx_usb_transfer_data(ucr,
> + usb_sndbulkpipe(ucr->pusb_dev, EP_BULK_OUT),
> + ucr->cmd_buf, ucr->cmd_idx * 4 + CMD_OFFSET,
> + 0, NULL, timeout);
> + if (ret) {
> + /* clear HW error*/
> + rtsx_usb_ep0_write_register(ucr, SFSM_ED, 0xf8, 0xf8);

Make this into a function and remove the comment.

rtsx_usb_ep0_clear_hw_error(ucr);

> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_send_cmd);
> +
> +int rtsx_usb_get_rsp(struct rtsx_ucr *ucr, int rsp_len, int timeout)
> +{
> + if (rsp_len <= 0)
> + return -EINVAL;
> +
> + if (rsp_len % 4)
> + rsp_len += (4 - rsp_len % 4);
> +
> + return rtsx_usb_transfer_data(ucr,
> + usb_rcvbulkpipe(ucr->pusb_dev, EP_BULK_IN),
> + ucr->rsp_buf, rsp_len, 0, NULL, timeout);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_get_rsp);
> +
> +static int rtsx_usb_get_status_with_bulk(struct rtsx_ucr *ucr, u16 *status)
> +{
> + int ret;
> +
> + rtsx_usb_init_cmd(ucr);
> + rtsx_usb_add_cmd(ucr, READ_REG_CMD, CARD_EXIST, 0x00, 0x00);
> + rtsx_usb_add_cmd(ucr, READ_REG_CMD, OCPSTAT, 0x00, 0x00);
> + ret = rtsx_usb_send_cmd(ucr, MODE_CR, 100);
> + if (ret)
> + return ret;
> +
> + ret = rtsx_usb_get_rsp(ucr, 2, 100);
> + if (ret)
> + return ret;
> +
> + *status = (u16)((ucr->rsp_buf[0] >> 2) & 0x0f) |
> + ((ucr->rsp_buf[1] & 0x03) << 4);

The cast to u16 is not needed. Align it like this:

*status = ((ucr->rsp_buf[0] >> 2) & 0x0f) |
((ucr->rsp_buf[1] & 0x03) << 4);

> +
> + return 0;
> +}
> +

[ snip ]

> +int rtsx_usb_switch_clock(struct rtsx_ucr *ucr, unsigned int card_clock,
> + u8 ssc_depth, bool initial_mode, bool double_clk, bool vpclk)
> +{
> + int ret;
> + u8 n, clk_divider, mcu_cnt, div;
> +
> + if (!card_clock) {
> + ucr->cur_clk = 0;
> + return 0;
> + }
> +
> + if (initial_mode) {
> + /* We use 250k(around) here, in initial stage */
> + clk_divider = SD_CLK_DIVIDE_128;
> + card_clock = 30000000;
> + } else {
> + clk_divider = SD_CLK_DIVIDE_0;
> + }
> +
> + ret = rtsx_usb_write_register(ucr, SD_CFG1,
> + SD_CLK_DIVIDE_MASK, clk_divider);
> + if (ret < 0)
> + return ret;
> +
> + card_clock /= 1000000;
> + dev_dbg(&ucr->pusb_intf->dev,
> + "Switch card clock to %dMHz\n", card_clock);
> +
> + if (!initial_mode && double_clk)
> + card_clock *= 2;
> + dev_dbg(&ucr->pusb_intf->dev,
> + "Internal SSC clock: %dMHz (cur_clk = %d)\n",
> + card_clock, ucr->cur_clk);
> +
> + if (card_clock == ucr->cur_clk)
> + return 0;
> +
> + /* Converting clock value into internal settings: n and div */
> + n = (u8)(card_clock - 2);

Unneeded cast.

> + if ((card_clock <= 2) || (n > MAX_DIV_N))
> + return -EINVAL;
> +
> + mcu_cnt = (u8)(60/card_clock + 3);

Unneeded cast. Obviously it fits in u8.

> + if (mcu_cnt > 15)
> + mcu_cnt = 15;
> +
> + /* Make sure that the SSC clock div_n is not less than MIN_DIV_N */
> +
> + div = CLK_DIV_1;
> + while ((n < MIN_DIV_N) && (div < CLK_DIV_4)) {

Unneeded parenthesis.

> + n = (n + 2) * 2 - 2;
> + div++;
> + }

[ snip ]

> +static int rtsx_usb_init_chip(struct rtsx_ucr *ucr)
> +{
> + int ret;
> + u8 val;
> +
> + /* clear HW error*/
> + rtsx_usb_ep0_write_register(ucr, SFSM_ED, 0xf8, 0xf8);
> +
> + /* power on SSC*/
> + ret = rtsx_usb_write_register(ucr,
> + FPDCTL, SSC_POWER_MASK, SSC_POWER_ON);
> + if (ret)
> + goto err_out;
> +
> + usleep_range(100, 1000);
> + ret = rtsx_usb_write_register(ucr, CLK_DIV, CLK_CHANGE, 0x00);
> + if (ret)
> + goto err_out;
> +
> + /* determine IC version */
> + ret = rtsx_usb_read_register(ucr, HW_VERSION, &val);
> + if (ret)
> + goto err_out;
> +
> + ucr->ic_version = val & HW_VER_MASK;
> +
> + /* determine package */
> + ret = rtsx_usb_read_register(ucr, CARD_SHARE_MODE, &val);
> + if (ret)
> + goto err_out;
> + if (val & CARD_SHARE_LQFP_SEL) {
> + ucr->package = LQFP48;
> + dev_dbg(&ucr->pusb_intf->dev, "Package: LQFP48\n");
> + } else {
> + ucr->package = QFN24;
> + dev_dbg(&ucr->pusb_intf->dev, "Package: QFN24\n");
> + }
> +
> + /* determine IC variations */
> + rtsx_usb_read_register(ucr, CFG_MODE_1, &val);
> + if (val & RTS5179) {
> + ucr->is_rts5179 = 1;
> + dev_dbg(&ucr->pusb_intf->dev, "Device is rts5179\n");
> + } else {
> + ucr->is_rts5179 = 0;
> + }
> +
> + ret = rtsx_usb_reset_chip(ucr);
> + if (ret)
> + goto err_out;
> +
> + return 0;
> +err_out:
> + return ret;

Gar... Just return directly instead of adding do-nothing-gotos... No
one likes skipping back and forth within a function to find what a goto
does and then it does nothing, it just returns.

> +}
> +
> +static int rtsx_usb_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *usb_dev = interface_to_usbdev(intf);
> + struct rtsx_ucr *ucr;
> + int i;
> + int ret;
> +
> + dev_dbg(&intf->dev,
> + ": Realtek USB Card Reader found at bus %03d address %03d\n",
> + usb_dev->bus->busnum, usb_dev->devnum);
> +
> + ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
> + if (!ucr) {
> + dev_err(&intf->dev, "Out of memory\n");

This printk is pointless. kzalloc() already has a much usefull
printk().

> + return -ENOMEM;
> + }
> +
> + ucr->pusb_dev = usb_dev;
> +
> + /* alloc coherent buffer */
> + ucr->iobuf = usb_alloc_coherent(ucr->pusb_dev, IOBUF_SIZE,
> + GFP_KERNEL, &ucr->iobuf_dma);
> + if (!ucr->iobuf) {
> + ret = -ENOMEM;
> + goto out_free_chip;
> + }
> +
> + /* set USB interface data */
> + usb_set_intfdata(intf, ucr);
> +
> + ucr->vendor_id = id->idVendor;
> + ucr->product_id = id->idProduct;
> + ucr->cmd_buf = ucr->rsp_buf = ucr->iobuf;
> +
> + mutex_init(&ucr->dev_mutex);
> +
> + ucr->pusb_intf = intf;
> +
> + /* initialize */
> + ret = rtsx_usb_init_chip(ucr);
> + if (ret)
> + goto out_init_fail;
> +
> + for (i = 0; i < ARRAY_SIZE(rtsx_usb_cells); i++) {
> + rtsx_usb_cells[i].platform_data = &ucr;
> + rtsx_usb_cells[i].pdata_size = sizeof(*ucr);
> + }
> + ret = mfd_add_devices(&intf->dev, usb_dev->devnum, rtsx_usb_cells,
> + ARRAY_SIZE(rtsx_usb_cells), NULL, 0, NULL);
> +

Don't put a blank line here.

> + if (ret)
> + goto out_init_fail;
> + /* initialize USB SG transfer timer */
> + init_timer(&ucr->sg_timer);
> + setup_timer(&ucr->sg_timer, rtsx_usb_sg_timed_out, (unsigned long) ucr);
> +#ifdef CONFIG_PM
> + intf->needs_remote_wakeup = 1;
> + usb_enable_autosuspend(usb_dev);
> +#endif
> +
> + return 0;
> +
> +out_init_fail:
> + usb_set_intfdata(ucr->pusb_intf, NULL);
> + usb_free_coherent(ucr->pusb_dev, IOBUF_SIZE, ucr->iobuf,
> + ucr->iobuf_dma);
> +
> +out_free_chip:
> + kfree(ucr);
> + dev_err(&intf->dev, "%s failed\n", __func__);
> + return ret;
> +}

[ snip ]

> +#define CARD_DMA1_CTL 0xFD5C
> +#define CARD_PULL_CTL1 0xFD60
> +#define CARD_PULL_CTL2 0xFD61
> +#define CARD_PULL_CTL3 0xFD62
> +#define CARD_PULL_CTL4 0xFD63
> +#define CARD_PULL_CTL5 0xFD64
> +#define CARD_PULL_CTL6 0xFD65
> +#define CARD_EXIST 0xFD6F

In the email CARD_EXIST looks aligned correctly, but in the actual .h
file then it's out of line.

> +#define CARD_INT_PEND 0xFD71
> +

[ snip ]

> +#define MC_IRQ 0xFF00
> +#define MC_IRQEN 0xFF01
> +#define MC_FIFO_CTL 0xFF02
> +#define MC_FIFO_BC0 0xFF03
> +#define MC_FIFO_BC1 0xFF04
> +#define MC_FIFO_STAT 0xFF05
> +#define MC_FIFO_MODE 0xFF06
> +#define MC_FIFO_RD_PTR0 0xFF07
> +#define MC_FIFO_RD_PTR1 0xFF08

MC_FIFO_RD_PTR0 and MC_FIFO_RD_PTR1 are not aligned correctly.

> +#define MC_DMA_CTL 0xFF10
> +#define MC_DMA_TC0 0xFF11

regards,
dan carpenter

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

Next message: Jiri Olsa: "[PATCHv2 1/2] tools lib traceevent: Replace tabs with spaces for allnon-commands statements"
Previous message: Jonathan Cameron: "Re: [PATCHv5 RESEND 4/4] iio: mxs-lradc: convert is_divided to a bitmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]