lkml.org 
[lkml]   [2012]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] memstick: add support for legacy memorysticks
    From
    Date
    On Wed, 2012-09-19 at 14:52 -0700, Andrew Morton wrote: 
    > On Wed, 19 Sep 2012 16:19:02 +0300
    > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
    >
    > > Based partially on MS standard spec quotes from Alex Dubov.
    > >
    > > As any code that works with user data this driver isn't
    > > recommended to use to write cards that contain valuable data.
    > >
    > > It tries its best though to avoid data corruption
    > > and possible damage to the card.
    > >
    > > Tested on MS DUO 64 MB card on Ricoh R592 card reader..
    > >
    >
    > Generally looks nice to me, although I wouldn't know a memstick if one
    > stuck to my shoe.
    >
    > I have a bunch of fairly trivial comments, and one head-scratcher for
    > Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?

    Awesome, thanks for the review!
    I will address all comments very soon.

    >
    > > ...
    > >
    > > +static int sg_nents(struct scatterlist *sg)
    > > +{
    > > + int nents = 0;
    > > + while (sg) {
    > > + nents++;
    > > + sg = sg_next(sg);
    > > + }
    > > +
    > > + return nents;
    > > +}
    >
    > I think we may as well put this in scatterlist.c immediately. It
    > wouldn't surprise me if we already have open-coded copies of this lying
    > around the tree.
    I agree, but as usual afraid that it wont be accepted here.

    >
    > > +/*
    > > + * Copies section of 'sg_from' starting from offset 'offset' and with length
    > > + * 'len' To another scatterlist of to_nents enties
    > > + */
    > > +static int sg_copy(struct scatterlist *sg_from, struct scatterlist *sg_to,
    > > + int to_nents, int offset, size_t len)
    >
    > The should return a size_t, to match the type of `len'. And the type
    > of `offset' is fishy - there's nothing which intrinsically limits these
    > things to 2G?
    In my case, I will never have large offsets here, but I agree with you
    completely.
    >
    > > +{
    > > + int copied = 0;
    > > +
    > > + while (offset > 0) {
    > > +
    > > + if (offset >= sg_from->length) {
    > > + if (sg_is_last(sg_from))
    > > + return 0;
    > > +
    > > + offset -= sg_from->length;
    > > + sg_from = sg_next(sg_from);
    > > + continue;
    > > + }
    > > +
    > > + copied = min(len, (size_t)(sg_from->length - offset));
    >
    > min_t would be more conventional. Or perhaps even min(), depending on
    > `offset's new type (but probably not).
    Agree.
    >
    > > + sg_set_page(sg_to, sg_page(sg_from),
    > > + copied, sg_from->offset + offset);
    > > +
    > > + len -= copied;
    > > + offset = 0;
    > > +
    > > + if (sg_is_last(sg_from) || !len)
    > > + goto out;
    > > +
    > > + sg_to = sg_next(sg_to);
    > > + to_nents--;
    > > + sg_from = sg_next(sg_from);
    > > + }
    > > +
    > > + while (len > sg_from->length && to_nents--) {
    > > +
    > > + len -= sg_from->length;
    > > + copied += sg_from->length;
    > > +
    > > + sg_set_page(sg_to, sg_page(sg_from),
    > > + sg_from->length, sg_from->offset);
    > > +
    > > + if (sg_is_last(sg_from) || !len)
    > > + goto out;
    > > +
    > > + sg_from = sg_next(sg_from);
    > > + sg_to = sg_next(sg_to);
    > > + }
    > > +
    > > + if (len && to_nents) {
    > > + sg_set_page(sg_to, sg_page(sg_from), len, sg_from->offset);
    > > + copied += len;
    > > + }
    > > +
    > > +out:
    > > + sg_mark_end(sg_to);
    > > + return copied;
    > > +}
    > > +
    > > +/*
    > > + * Compares section of 'sg' starting from offset 'offset' and with length 'len'
    > > + * to linear buffer of length 'len' at address 'buffer'
    >
    > The comment should document the return value.
    >
    > > + */
    > > +static bool sg_compare_to_buffer(struct scatterlist *sg,
    > > + int offset, u8 *buffer, size_t len)
    > > +{
    > > + unsigned long flags;
    > > + int retval = 0;
    > > + struct sg_mapping_iter miter;
    > > +
    > > + local_irq_save(flags);
    >
    > hm, the "IRQ disabled if SG_MITER_ATOMIC" requirement of
    > sg_miter_next() is weird and unexpected.
    >
    > Tejun's 137d3edb48425f8 added the code whch has this strange
    > requirement, but it is unexplained. Wazzup?
    >
    > > + sg_miter_start(&miter, sg, sg_nents(sg),
    > > + SG_MITER_ATOMIC | SG_MITER_FROM_SG);
    > > +
    > > + while (sg_miter_next(&miter) && len > 0) {
    > > +
    > > + int cmplen;
    > > +
    > > + if (offset >= miter.length) {
    > > + offset -= miter.length;
    > > + continue;
    > > + }
    > > +
    > > + cmplen = min(miter.length - offset, len);
    > > + retval = memcmp(miter.addr + offset, buffer, cmplen);
    > > + if (retval)
    > > + break;
    > > +
    > > + buffer += cmplen;
    > > + len -= cmplen;
    > > + offset = 0;
    > > + }
    > > +
    > > + if (!retval && len)
    > > + retval = -1;
    > > +
    > > + sg_miter_stop(&miter);
    > > + local_irq_restore(flags);
    > > + return retval;
    > > +}
    > >
    > > ...
    > >
    > > +static void msb_mark_block_unused(struct msb_data *msb, int pba)
    > > +{
    > > + int zone = msb_get_zone_from_pba(pba);
    > > +
    > > + if (!test_bit(pba, msb->used_blocks_bitmap)) {
    > > + ms_printk("BUG: attempt to mark "
    > > + "already unused pba %d as unused" , pba);
    > > + msb->read_only = true;
    > > + return;
    > > + }
    > > +
    > > +#ifdef DEBUG
    > > + if (msb_validate_used_block_bitmap(msb))
    > > + return;
    > > +#endif
    > > + clear_bit(pba, msb->used_blocks_bitmap);
    > > + msb->free_block_count[zone]++;
    > > +}
    >
    > Presumably there is some lock which the caller must hold to prevent the
    > obvious races in the above two functions. I suggest adding a comment
    > whcih documents this requirement, then take advantage of that lock to
    > switch to the more efficient __set_bit() and __clear_bit().
    There can't be races in the driver, since it contains a single thread
    that does all the IO it got from block layer.
    The thread is awaken each time the request function of block device is
    called.
    This why I didn't do much locking in here. In addition I found out that
    this is quite common way to implement a block device driver.

    >
    > > +/* Invalidate current register window*/
    >
    > hm, checkpatch misses the missing space before */
    >
    > > +static void msb_invalidate_reg_window(struct msb_data *msb)
    > > +{
    > > + msb->reg_addr.w_offset = offsetof(struct ms_register, id);
    > > + msb->reg_addr.w_length = sizeof(struct ms_id_register);
    > > + msb->reg_addr.r_offset = offsetof(struct ms_register, id);
    > > + msb->reg_addr.r_length = sizeof(struct ms_id_register);
    > > + msb->addr_valid = false;
    > > +}
    > > +
    > > +/* Sane way to start a state machine*/
    > > +static int msb_run_state_machine(struct msb_data *msb, int (*state_func)
    > > + (struct memstick_dev *card, struct memstick_request **req))
    > > +{
    > > + struct memstick_dev *card = msb->card;
    > > +
    > > + WARN_ON(msb->state != -1);
    > > + msb->int_polling = false;
    > > + msb->state = 0;
    > > + msb->exit_error = 0;
    > > +
    > > + memset(&card->current_mrq, 0, sizeof(card->current_mrq));
    > > +
    > > + card->next_request = state_func;
    > > + memstick_new_req(card->host);
    > > + wait_for_completion(&card->mrq_complete);
    > > +
    > > + WARN_ON(msb->state != -1);
    > > + return msb->exit_error;
    > > +}
    > > +
    > > +/* State machines call that to exit */
    > > +int msb_exit_state_machine(struct msb_data *msb, int error)
    >
    > static
    >
    > > +{
    > > + WARN_ON(msb->state == -1);
    > > +
    > > + msb->state = -1;
    > > + msb->exit_error = error;
    > > + msb->card->next_request = h_msb_default_bad;
    > > +
    > > + /* Invalidate reg window on errors */
    > > + if (error)
    > > + msb_invalidate_reg_window(msb);
    > > +
    > > + complete(&msb->card->mrq_complete);
    > > + return -ENXIO;
    > > +}
    > > +
    > > +/* read INT register */
    > > +int msb_read_int_reg(struct msb_data *msb, long timeout)
    >
    > static
    >
    > > +{
    > > + struct memstick_request *mrq = &msb->card->current_mrq;
    > > + WARN_ON(msb->state == -1);
    >
    > It's conventional to put a newline between end-of-locals and start-of-code.
    >
    > > + if (!msb->int_polling) {
    > > + msb->int_timeout = jiffies +
    > > + msecs_to_jiffies(timeout == -1 ? 500 : timeout);
    > > + msb->int_polling = true;
    > > + } else if (time_after(jiffies, msb->int_timeout)) {
    > > + mrq->data[0] = MEMSTICK_INT_CMDNAK;
    > > + return 0;
    > > + }
    > > +
    > > + if ((msb->caps & MEMSTICK_CAP_AUTO_GET_INT) &&
    > > + mrq->need_card_int && !mrq->error) {
    > > + mrq->data[0] = mrq->int_reg;
    > > + mrq->need_card_int = false;
    > > + return 0;
    > > + } else {
    > > + memstick_init_req(mrq, MS_TPC_GET_INT, NULL, 1);
    > > + return 1;
    > > + }
    > > +}
    > > +
    > > +/* Read a register */
    > > +int msb_read_regs(struct msb_data *msb, int offset, int len)
    >
    > static. I'll stop checking them now ;)
    >
    > > +{
    > > + struct memstick_request *req = &msb->card->current_mrq;
    > > +
    > > + if (msb->reg_addr.r_offset != offset ||
    > > + msb->reg_addr.r_length != len || !msb->addr_valid) {
    > > +
    > > + msb->reg_addr.r_offset = offset;
    > > + msb->reg_addr.r_length = len;
    > > + msb->addr_valid = true;
    > > +
    > > + memstick_init_req(req, MS_TPC_SET_RW_REG_ADRS,
    > > + &msb->reg_addr, sizeof(msb->reg_addr));
    > > + return 0;
    > > + }
    > > +
    > > + memstick_init_req(req, MS_TPC_READ_REG, NULL, len);
    > > + return 1;
    > > +}
    > > +
    > > +/* Write a card register */
    > > +int msb_write_regs(struct msb_data *msb, int offset, int len, void *buf)
    > > +{
    > > + struct memstick_request *req = &msb->card->current_mrq;
    > > +
    > > + if (msb->reg_addr.w_offset != offset ||
    > > + msb->reg_addr.w_length != len || !msb->addr_valid) {
    > > +
    > > + msb->reg_addr.w_offset = offset;
    > > + msb->reg_addr.w_length = len;
    > > + msb->addr_valid = true;
    > > +
    > > + memstick_init_req(req, MS_TPC_SET_RW_REG_ADRS,
    > > + &msb->reg_addr, sizeof(msb->reg_addr));
    > > + return 0;
    > > + }
    > > +
    > > + memstick_init_req(req, MS_TPC_WRITE_REG, buf, len);
    > > + return 1;
    > > +}
    > > +
    > > +/* Handler for absense of IO */
    >
    > "absence" :)
    >
    > > +static int h_msb_default_bad(struct memstick_dev *card,
    > > + struct memstick_request **mrq)
    > > +{
    > > + return -ENXIO;
    > > +}
    > > +
    > > +/*
    > > + * This function is a handler for reads of one page from device.
    > > + * Writes output to msb->current_sg, takes sector address from msb->reg.param
    > > + * Can also be used to read extra data only. Set params accordintly.
    > > + */
    > > +static int h_msb_read_page(struct memstick_dev *card,
    > > + struct memstick_request **out_mrq)
    > > +{
    > > + struct msb_data *msb = memstick_get_drvdata(card);
    > > + struct memstick_request *mrq = *out_mrq = &card->current_mrq;
    > > + struct scatterlist sg[2];
    > > + u8 command, intreg;
    > > +
    > > + if (mrq->error) {
    > > + dbg("read_page, unknown error");
    > > + return msb_exit_state_machine(msb, mrq->error);
    > > + }
    > > +again:
    > > + switch (msb->state) {
    > > + case 0: /* Write the sector address */
    > > + if (!msb_write_regs(msb,
    > > + offsetof(struct ms_register, param),
    > > + sizeof(struct ms_param_register),
    > > + (unsigned char *)&msb->regs.param))
    > > + return 0;
    > > + break;
    > > +
    > > + case 1: /* Execute the read command*/
    > > + command = MS_CMD_BLOCK_READ;
    > > + memstick_init_req(mrq, MS_TPC_SET_CMD, &command, 1);
    > > + break;
    > > +
    > > + case 2: /* send INT request */
    > > + if (msb_read_int_reg(msb, -1))
    > > + break;
    > > + msb->state++;
    > > +
    > > + case 3: /* get result of the INT request*/
    > > + intreg = mrq->data[0];
    > > + msb->regs.status.interrupt = intreg;
    > > +
    > > + if (intreg & MEMSTICK_INT_CMDNAK)
    > > + return msb_exit_state_machine(msb, -EIO);
    > > +
    > > + if (!(intreg & MEMSTICK_INT_CED)) {
    > > + msb->state--;
    > > + goto again;
    > > + }
    > > +
    > > + msb->int_polling = false;
    > > +
    > > + if (intreg & MEMSTICK_INT_ERR)
    > > + msb->state++;
    > > + else
    > > + msb->state = 6;
    > > +
    > > + goto again;
    > > +
    > > + case 4: /* read the status register
    > > + to understand source of the INT_ERR */
    > > + if (!msb_read_regs(msb,
    > > + offsetof(struct ms_register, status),
    > > + sizeof(struct ms_status_register)))
    > > + return 0;
    > > + break;
    > > +
    > > + case 5: /* get results of status check */
    > > + msb->regs.status = *(struct ms_status_register *)mrq->data;
    > > + msb->state++;
    > > +
    > > + case 6: /* Send extra data read request */
    > > + if (!msb_read_regs(msb,
    > > + offsetof(struct ms_register, extra_data),
    > > + sizeof(struct ms_extra_data_register)))
    > > + return 0;
    > > + break;
    > > +
    > > + case 7: /* Save result of extra data request */
    > > + msb->regs.extra_data =
    > > + *(struct ms_extra_data_register *) mrq->data;
    > > + msb->state++;
    > > +
    > > + case 8: /* Send the MS_TPC_READ_LONG_DATA to read IO buffer */
    > > +
    > > + /* Skip that state if we only read the oob */
    > > + if (msb->regs.param.cp == MEMSTICK_CP_EXTRA) {
    > > + msb->state++;
    > > + goto again;
    > > + }
    > > +
    > > + sg_init_table(sg, ARRAY_SIZE(sg));
    > > + sg_copy(msb->current_sg, sg, ARRAY_SIZE(sg),
    > > + msb->current_sg_offset,
    > > + msb->page_size);
    > > +
    > > + memstick_init_req_sg(mrq, MS_TPC_READ_LONG_DATA, sg);
    > > + break;
    > > +
    > > + case 9: /* check validity of data buffer & done */
    > > +
    > > + if (!(msb->regs.status.interrupt & MEMSTICK_INT_ERR)) {
    > > + msb->current_sg_offset += msb->page_size;
    > > + return msb_exit_state_machine(msb, 0);
    > > + }
    > > +
    > > + if (msb->regs.status.status1 & MEMSTICK_UNCORR_ERROR) {
    > > + dbg("read_page: uncorrectable error");
    > > + return msb_exit_state_machine(msb, -EBADMSG);
    > > + }
    > > +
    > > + if (msb->regs.status.status1 & MEMSTICK_CORR_ERROR) {
    > > + dbg("read_page: correctable error");
    > > + msb->current_sg_offset += msb->page_size;
    > > + return msb_exit_state_machine(msb, -EUCLEAN);
    > > + } else {
    > > + dbg("read_page: INT error, but no status error bits");
    > > + return msb_exit_state_machine(msb, -EIO);
    > > + }
    > > + default:
    > > + BUG();
    > > + }
    > > + msb->state++;
    >
    > I think this code would be quite a lot cleaner, safer and more readable
    > if you were to do away with the "msb->state++" concept altogether. Just do
    >
    > enum msb_states {
    > state_foo1,
    > state_foo2
    > ...
    > };
    >
    > and do
    >
    > msb->state = state_foo2;
    >
    > in each switch case.
    >
    > This way, the reader doesn't have to scratch his head over what the
    > state _used_ to be, and the states have nice names to help the reader
    > understand what's going on.
    I now remember hearing about that back then too, and on second thought,
    I'll do that.

    >
    >
    > > + return 0;
    > > +}
    > >
    > > ...
    > >
    > > +/*
    > > + * This function is used to send simple IO requests to device that consist
    > > + * of register write + command
    > > + */
    > > +static int h_msb_send_command(struct memstick_dev *card,
    > > + struct memstick_request **out_mrq)
    > > +{
    > > + struct msb_data *msb = memstick_get_drvdata(card);
    > > + struct memstick_request *mrq = *out_mrq = &card->current_mrq;
    > > +
    >
    > stray newline
    >
    > > + u8 intreg;
    > > +
    > > + if (mrq->error) {
    > > + dbg("send_command: unknown error");
    > > + return msb_exit_state_machine(msb, mrq->error);
    > > + }
    > > +again:
    > > + switch (msb->state) {
    > > +
    > > + /* HACK: see h_msb_write_block */
    > > +
    > >
    > > ...
    > >
    > > +static int h_msb_parallel_switch(struct memstick_dev *card,
    > > + struct memstick_request **out_mrq)
    > > +{
    > > + struct msb_data *msb = memstick_get_drvdata(card);
    > > + struct memstick_request *mrq = *out_mrq = &card->current_mrq;
    > > +
    >
    > stray newline. Please fix 'em up - it does make the code a little
    > discordant to read.
    >
    > > + struct memstick_host *host = card->host;
    > > +
    > > + if (mrq->error) {
    > > + dbg("parallel_switch: error");
    > > + msb->regs.param.system &= ~MEMSTICK_SYS_PAM;
    > > + return msb_exit_state_machine(msb, mrq->error);
    > > + }
    > > +
    > > + switch (msb->state) {
    > > + case 0: /* Set the parallel interface on memstick side */
    > > + msb->regs.param.system |= MEMSTICK_SYS_PAM;
    > > +
    > > + if (!msb_write_regs(msb,
    > > + offsetof(struct ms_register, param),
    > > + 1,
    > > + (unsigned char *)&msb->regs.param))
    > > + return 0;
    > > + break;
    > > +
    > > + case 1: /* Set parallel interface on our side + send a dummy request
    > > + to see if card responds */
    > > + host->set_param(host, MEMSTICK_INTERFACE, MEMSTICK_PAR4);
    > > + memstick_init_req(mrq, MS_TPC_GET_INT, NULL, 1);
    > > + break;
    > > +
    > > + case 2:
    > > + return msb_exit_state_machine(msb, 0);
    > > + }
    > > + msb->state++;
    > > + return 0;
    > > +}
    > > +
    > > +static int msb_switch_to_parallel(struct msb_data *msb);
    > > +
    > > +/* Reset the card, to guard against hw errors beeing treated as bad blocks */
    > > +static int msb_reset(struct msb_data *msb, bool full)
    > > +{
    > > +
    > > + bool was_parallel = msb->regs.param.system & MEMSTICK_SYS_PAM;
    >
    > fyi, alas, this is (probably) slower than using an `int'. gcc will
    > convert the result of the expression into 1-or-0.
    Didn't knew about that. Here this code is executed once per card insert,
    so it sure doesn't matter.
    >
    > > + struct memstick_dev *card = msb->card;
    > > + struct memstick_host *host = card->host;
    > > + int error;
    > > +
    > > + /* Reset the card */
    > > + msb->regs.param.system = MEMSTICK_SYS_BAMD;
    > > +
    > > + if (full) {
    > > + error = host->set_param(host,
    > > + MEMSTICK_POWER, MEMSTICK_POWER_OFF);
    > > + if (error)
    > > + goto out_error;
    > > +
    > > + msb_invalidate_reg_window(msb);
    > > +
    > > + error = host->set_param(host,
    > > + MEMSTICK_POWER, MEMSTICK_POWER_ON);
    > > + if (error)
    > > + goto out_error;
    > > +
    > > + error = host->set_param(host,
    > > + MEMSTICK_INTERFACE, MEMSTICK_SERIAL);
    > > + if (error) {
    > >
    > > ...
    > >
    > > +static int msb_erase_block(struct msb_data *msb, u16 pba)
    > > +{
    > > + int error, try;
    > > + if (msb->read_only)
    > > + return -EROFS;
    > > +
    > > + dbg_verbose("erasing pba %d", pba);
    > > +
    > > + for (try = 1 ; try < 3 ; try++) {
    > > + msb->regs.param.block_address = cpu_to_be16(pba);
    > > + msb->regs.param.page_address = 0;
    > > + msb->regs.param.cp = MEMSTICK_CP_BLOCK;
    > > + msb->command_value = MS_CMD_BLOCK_ERASE;
    > > + msb->command_need_oob = false;
    > > +
    > > +
    > > + error = msb_run_state_machine(msb, h_msb_send_command);
    > > + if (!error || msb_reset(msb, true))
    > > + break;
    > > + }
    > > +
    > > + if (error) {
    > > + ms_printk("erase failed, marking pba %d as bad", pba);
    > > + msb_mark_bad(msb, pba);
    > > + }
    > > +
    > > + dbg_verbose("erase success, marking pba %d as unused", pba);
    > > + msb_mark_block_unused(msb, pba);
    > > + set_bit(pba, msb->erased_blocks_bitmap);
    >
    > Should be able to use __set_bit() here, as msb_mark_block_unused()
    > required caller-provided locking.
    Again, I think I don't need locking here.
    >
    > > + return error;
    > > +}
    > > +
    > >
    > > ...
    > >
    > > +static int msb_read_oob(struct msb_data *msb, u16 pba, u16 page,
    > > + struct ms_extra_data_register *extra)
    > > +{
    > > + int error;
    > > + BUG_ON(!extra);
    > > +
    >
    > vertical whitespace oddnesses
    >
    > > + msb->regs.param.block_address = cpu_to_be16(pba);
    > > + msb->regs.param.page_address = page;
    > > + msb->regs.param.cp = MEMSTICK_CP_EXTRA;
    > > +
    > > + if (pba > msb->block_count) {
    > > + ms_printk("BUG: attempt to read beyond"
    > > + " the end of card at pba %d", pba);
    > > + return -EINVAL;
    > > + }
    > > +
    > > + error = msb_run_state_machine(msb, h_msb_read_page);
    > > + *extra = msb->regs.extra_data;
    > > +
    > > + if (error == -EUCLEAN) {
    > > + ms_printk("correctable error on pba %d, page %d",
    > > + pba, page);
    > > + return 0;
    > > + }
    > > +
    > > + return error;
    > > +}
    > > +
    > > +
    > > +/* Reads a block and compares it with data contained in scatterlist orig_sg */
    > > +static bool msb_verify_block(struct msb_data *msb, u16 pba,
    > > + struct scatterlist *orig_sg, int offset)
    > > +{
    > > + struct scatterlist sg;
    > > + int page = 0, error;
    > > +
    > > + sg_init_one(&sg, msb->block_buffer, msb->block_size);
    > > +
    > > + while (page < msb->pages_in_block) {
    > > +
    > > + error = msb_read_page(msb, pba, page,
    > > + NULL, &sg, page * msb->page_size);
    > > + if (error)
    > > + return -EIO;
    >
    > msb_read_page() returned an errno. Propagate that, rather than
    > overwriting it?
    Sure!
    >
    > > + page++;
    > > + }
    > > +
    > > + if (sg_compare_to_buffer(orig_sg, offset,
    > > + msb->block_buffer, msb->block_size))
    > > + return -EIO;
    > > + return 0;
    > > +}
    > > +
    > >
    > > ...
    > >
    > > +static int msb_read_bad_block_table(struct msb_data *msb, int block_nr)
    >
    > hm. Should block_nr be a sector_t?
    Actually no, as its an index in internal array of 'boot blocks', aka
    superblocks and there should be just two of them
    msb_read_boot_blocks reads them.
    Each of boot blocks contains a pointer to bad blocks table.
    >
    >
    > > +{
    > > + struct ms_boot_page *boot_block;
    > > + struct scatterlist sg;
    > > + u16 *buffer = NULL;
    > > + int offset = 0;
    > > +
    > > + int i, error = 0;
    > > + int data_size, data_offset, page, page_offset, size_to_read;
    > > + u16 pba;
    >
    > vertical whitespace oddness
    >
    > > + BUG_ON(block_nr > 1);
    > > +
    > > + boot_block = &msb->boot_page[block_nr];
    > > + pba = msb->boot_block_locations[block_nr];
    > > +
    > >
    > > ...
    > >
    > > +static int msb_cache_write(struct msb_data *msb, int lba,
    > > + int page, bool add_to_cache_only, struct scatterlist *sg, int offset)
    > > +{
    > > + int error;
    > > + struct scatterlist sg_tmp[10];
    >
    > newline..
    >
    > > + if (msb->read_only)
    > > + return -EROFS;
    > > +
    > > + if (msb->cache_block_lba == MS_BLOCK_INVALID ||
    > > + lba != msb->cache_block_lba)
    > > + if (add_to_cache_only)
    > > + return 0;
    > > +
    > > + /* If we need to write different block */
    > > + if (msb->cache_block_lba != MS_BLOCK_INVALID &&
    > > + lba != msb->cache_block_lba) {
    > > + dbg_verbose("first flush the cache");
    > > + error = msb_cache_flush(msb);
    > > + if (error)
    > > + return error;
    > > + }
    > > +
    > > + if (msb->cache_block_lba == MS_BLOCK_INVALID) {
    > > + msb->cache_block_lba = lba;
    > > + mod_timer(&msb->cache_flush_timer,
    > > + jiffies + msecs_to_jiffies(cache_flush_timeout));
    > > + }
    > > +
    > > + dbg_verbose("Write of LBA %d page %d to cache ", lba, page);
    > > +
    > > + sg_init_table(sg_tmp, ARRAY_SIZE(sg_tmp));
    > > + sg_copy(sg, sg_tmp, ARRAY_SIZE(sg_tmp), offset, msb->page_size);
    > > +
    > > + sg_copy_to_buffer(sg_tmp, sg_nents(sg_tmp),
    > > + msb->cache + page * msb->page_size, msb->page_size);
    > > +
    > > + set_bit(page, &msb->valid_cache_bitmap);
    > > + return 0;
    > > +}
    > > +
    > >
    > > ...
    > >
    > > +static int msb_do_write_request(struct msb_data *msb, int lba,
    > > + int page, struct scatterlist *sg, int len, int *sucessfuly_written)
    > > +{
    > > + int error = 0;
    > > + int offset = 0;
    > > + *sucessfuly_written = 0;
    >
    > (more vertical whitespace glitches)
    >
    > Again the types look a bit wrong. I'd have expected `len' and `offset'
    > to be size_t.
    >
    > > + while (offset < len) {
    > > + if (page == 0 && len - offset >= msb->block_size) {
    > > +
    > > + if (msb->cache_block_lba == lba)
    > > + msb_cache_discard(msb);
    > > +
    > > + dbg_verbose("Writing whole lba %d", lba);
    > > + error = msb_update_block(msb, lba, sg, offset);
    > > + if (error)
    > > + return error;
    > > +
    > > + offset += msb->block_size;
    > > + *sucessfuly_written += msb->block_size;
    > > + lba++;
    > > + continue;
    > > + }
    > > +
    > > + error = msb_cache_write(msb, lba, page, false, sg, offset);
    > > + if (error)
    > > + return error;
    > > +
    > > + offset += msb->page_size;
    > > + *sucessfuly_written += msb->page_size;
    > > +
    > > + page++;
    > > + if (page == msb->pages_in_block) {
    > > + page = 0;
    > > + lba++;
    > > + }
    > > + }
    > > + return 0;
    > > +}
    > >
    > > ...
    > >
    > > +static DEFINE_IDR(msb_disk_idr);
    > > +static DEFINE_MUTEX(msb_disk_lock);
    >
    > A little comment which describes the overall role of msb_disk_idr wold
    > be nice.
    Sure!

    The things I didn't comment on I agree too, I will fix all these
    non-static functions and newlines. Newlines I put mostly to make the
    code a bit more clear to myself, but I really don't mind removing these.

    Thanks again for a review, I will address all the issues very soon.

    --
    Best regards,
    Maxim Levitsky



    \
     
     \ /
      Last update: 2012-09-20 06:41    [W:0.082 / U:35.356 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site