Date: Wed, 22 Mar 2006 13:28:34 -0800
From: Andrew Morton <akpm@osdl.org>
To: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: shbader@de.ibm.com, holzheu@de.ibm.com, linux-kernel@vger.kernel.org
Subject: Re: [patch 20/24] s390: 3590 tape driver
Message-Id: <20060322132834.051a3dc4.akpm@osdl.org>
In-Reply-To: <20060322152547.GT5801@skybase.boeblingen.de.ibm.com>
References: <20060322152547.GT5801@skybase.boeblingen.de.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3422
Lines: 127

Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
>
> From: Stefan Bader <shbader@de.ibm.com>
> From: Michael Holzheu <holzheu@de.ibm.com>
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/