Martin Schwidefsky <[email protected]> wrote:
>
> From: Stefan Bader <[email protected]>
> From: Michael Holzheu <[email protected]>
> From: Martin Schwidefsky <[email protected]>
>
> [patch 20/24] s390: 3590 tape driver
>
> ...
>
> +static void
> +tape_3590_work_handler(void *data)
> +{
> + struct {
> + struct tape_device *device;
> + enum tape_op op;
> + struct work_struct work;
> + } *p = data;
> +
> + switch (p->op) {
> + case TO_MSEN:
> + tape_3590_sense_medium(p->device);
> + break;
> + case TO_READ_ATTMSG:
> + tape_3590_read_attmsg(p->device);
> + break;
> + default:
> + DBF_EVENT(3, "T3590: work handler undefined for "
> + "operation 0x%02x\n", p->op);
> + }
> + tape_put_device(p->device);
> + kfree(p);
> +}
> +
> +static int
> +tape_3590_schedule_work(struct tape_device *device, enum tape_op op)
> +{
> + struct {
> + struct tape_device *device;
> + enum tape_op op;
> + struct work_struct work;
> + } *p;
> +
> + if ((p = kzalloc(sizeof(*p), GFP_ATOMIC)) == NULL)
> + return -ENOMEM;
> +
> + INIT_WORK(&p->work, tape_3590_work_handler, p);
> +
> + p->device = tape_get_device_reference(device);
> + p->op = op;
> +
> + schedule_work(&p->work);
> + return 0;
> +}
That duplicated struct definition is pretty poor form, IMO. Best to have a
single definition.
What happens to this driver if the GFP_ATOMIC allocation fails?
I don't see a flush_scheduled_work() in this driver. Is it not possible
that there's still work pending at shutdown or rmmod time?
> +#ifdef CONFIG_S390_TAPE_BLOCK
> +/*
> + * Tape Block READ
> + */
> +static struct tape_request *
> +tape_3590_bread(struct tape_device *device, struct request *req)
> +{
> + struct tape_request *request;
> + struct ccw1 *ccw;
> + int count = 0, start_block, i;
> + unsigned off;
> + char *dst;
> + struct bio_vec *bv;
> + struct bio *bio;
> +
> + DBF_EVENT(6, "xBREDid:");
> + start_block = req->sector >> TAPEBLOCK_HSEC_S2B;
> + DBF_EVENT(6, "start_block = %i\n", start_block);
> +
> + rq_for_each_bio(bio, req) {
> + bio_for_each_segment(bv, bio, i) {
> + count += bv->bv_len >> (TAPEBLOCK_HSEC_S2B + 9);
> + }
> + }
> + request = tape_alloc_request(2 + count + 1, 4);
> + if (IS_ERR(request))
> + return request;
> + request->op = TO_BLOCK;
> + *(__u32 *) request->cpdata = start_block;
> + ccw = request->cpaddr;
> + ccw = tape_ccw_cc(ccw, MODE_SET_DB, 1, device->modeset_byte);
> +
> + /*
> + * We always setup a nop after the mode set ccw. This slot is
> + * used in tape_std_check_locate to insert a locate ccw if the
> + * current tape position doesn't match the start block to be read.
> + */
> + ccw = tape_ccw_cc(ccw, NOP, 0, NULL);
> +
> + rq_for_each_bio(bio, req) {
> + bio_for_each_segment(bv, bio, i) {
> + dst = kmap(bv->bv_page) + bv->bv_offset;
> + for (off = 0; off < bv->bv_len;
> + off += TAPEBLOCK_HSEC_SIZE) {
> + ccw->flags = CCW_FLAG_CC;
> + ccw->cmd_code = READ_FORWARD;
> + ccw->count = TAPEBLOCK_HSEC_SIZE;
> + set_normalized_cda(ccw, (void *) __pa(dst));
> + ccw++;
> + dst += TAPEBLOCK_HSEC_SIZE;
> + }
> + if (off > bv->bv_len)
> + BUG();
> + }
> + }
We seem to be missing a kunmap().
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
