>
> > + case UPIU_TRANSACTION_QUERY_REQ:
> > + qr = &bsg_request->upiu_req.qr;
> > + if (qr->opcode == UPIU_QUERY_OPCODE_READ_DESC)
> > + goto not_supported;
> > +
> > + if (ufs_bsg_get_query_desc_size(hba, qr, &desc_len))
> > + goto null_desc_buff;
> > +
> > + if (qr->opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> > + rw = WRITE;
> > + desc_buff = (uint8_t *)(bsg_request + 1);
> > + }
> > +
> > +null_desc_buff:
> > + /* fall through */
> > + case UPIU_TRANSACTION_NOP_OUT:
> > + case UPIU_TRANSACTION_TASK_REQ:
> > + /* Now that we know if its a read or write, verify again */
> > + if (rw != UFS_BSG_NOP || desc_len) {
> > + ret = ufs_bsg_verify_query_size(request_len,
> reply_len,
> > + rw, desc_len);
> > + if (ret) {
> > + dev_err(job->dev,
> > + "not enough space assigned\n");
> > + goto out;
> > + }
> > + }
>
> I think this check should be moved into the above switch case
> as it can only be hit for UPIU_TRANSACTION_QUERY_REQ requests. In
> fact I think the code would be a lot cleaner if you could factor
> the UPIU_TRANSACTION_QUERY_REQ case into a little helper function
> (ufs_bsg_get_query_desc_size should probably be merged into that
> helper also).
Done.
>
> > + case UPIU_TRANSACTION_UIC_CMD:
This should introduced only in the next patch
> > + /* later */
> > + case UPIU_TRANSACTION_COMMAND:
> > + case UPIU_TRANSACTION_DATA_OUT:
> > +not_supported:
> > + /* for the time being, we do not support data transfer upiu */
> > + ret = -ENOTSUPP;
> > + dev_err(job->dev, "unsupported msgcode 0x%x\n", msgcode);
> > +
> > + break;
> > + default:
> > + ret = -EINVAL;
> > +
> > + break;
> > + }
>
> This difference between known but not supported and not recognized is
> a bit odd. I think there should be just the generic not supported
> case, and you can decide if you want to log it or not.
Done.
>
> Also please try to avoid goto labels jumping into switch statements,
> code is generlaly a lot more readable if you keep the goto targets
> outside the switch statement.
Done.
Thanks,
Avri
