(unknown)

(unknown)

[FUJITA Tomonori]

This patchset fixes page leak issue in discard commands with unprep
facility that James posted:

http://marc.info/?l=linux-scsi&m=127791727508214&w=2

The 1/3 patch adds unprep facility to the block layer (identical to
what James posted).

The 2/3 patch frees a page for discard commands by using the unprep
facility. James' original patch doesn't work since it accesses to
rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is
called when all the data buffer (req->bio and scsi_data_buffer) in the
request is freed.

I use rq->buffer to keep track of an allocated page as the block layer
sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't
use rq->buffer (rq->buffer is set to NULL). So I can't say that I like
it lots. Any other way to do that?

The 3/3 path just removes the dead code.

This is against Jens' for-2.6.36.

The git tree is also available:

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git unprep

I'll update the discard FS request conversion on the top of this soon. But this can be applied independently (and fixes the memory leak).

=
 block/blk-core.c        |   25 +++++++++++++++++++++++++
 block/blk-settings.c    |   17 +++++++++++++++++
 drivers/scsi/scsi_lib.c |    2 +-
 drivers/scsi/sd.c       |   25 +++++++++++++++----------
 include/linux/blkdev.h  |    4 ++++
 5 files changed, 62 insertions(+), 11 deletions(-)

[PATCH 1/3] block: implement an unprep function corresponding directly to prep

[FUJITA Tomonori]

From: James Bottomley <James.Bottomley@suse.de>

Reviewed-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 block/blk-core.c        |   25 +++++++++++++++++++++++++
 block/blk-settings.c    |   17 +++++++++++++++++
 drivers/scsi/scsi_lib.c |    2 +-
 include/linux/blkdev.h  |    4 ++++
 4 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3c37894..5ab3ac2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -608,6 +608,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
 
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
+	q->unprep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
 	q->queue_flags		= QUEUE_FLAG_DEFAULT;
 	q->queue_lock		= lock;
@@ -2133,6 +2134,26 @@ static bool blk_update_bidi_request(struct request *rq, int error,
 	return false;
 }
 
+/**
+ * blk_unprep_request - unprepare a request
+ * @req:	the request
+ *
+ * This function makes a request ready for complete resubmission (or
+ * completion).  It happens only after all error handling is complete,
+ * so represents the appropriate moment to deallocate any resources
+ * that were allocated to the request in the prep_rq_fn.  The queue
+ * lock is held when calling this.
+ */
+void blk_unprep_request(struct request *req)
+{
+	struct request_queue *q = req->q;
+
+	req->cmd_flags &= ~REQ_DONTPREP;
+	if (q->unprep_rq_fn)
+		q->unprep_rq_fn(q, req);
+}
+EXPORT_SYMBOL_GPL(blk_unprep_request);
+
 /*
  * queue lock must be held
  */
@@ -2148,6 +2169,10 @@ static void blk_finish_request(struct request *req, int error)
 
 	blk_delete_timer(req);
 
+	if (req->cmd_flags & REQ_DONTPREP)
+		blk_unprep_request(req);
+
+
 	blk_account_io_done(req);
 
 	if (req->end_io)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index f5ed5a1..a234f4b 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -37,6 +37,23 @@ void blk_queue_prep_rq(struct request_queue *q, prep_rq_fn *pfn)
 EXPORT_SYMBOL(blk_queue_prep_rq);
 
 /**
+ * blk_queue_unprep_rq - set an unprepare_request function for queue
+ * @q:		queue
+ * @ufn:	unprepare_request function
+ *
+ * It's possible for a queue to register an unprepare_request callback
+ * which is invoked before the request is finally completed. The goal
+ * of the function is to deallocate any data that was allocated in the
+ * prepare_request callback.
+ *
+ */
+void blk_queue_unprep_rq(struct request_queue *q, unprep_rq_fn *ufn)
+{
+	q->unprep_rq_fn = ufn;
+}
+EXPORT_SYMBOL(blk_queue_unprep_rq);
+
+/**
  * blk_queue_merge_bvec - set a merge_bvec function for queue
  * @q:		queue
  * @mbfn:	merge_bvec_fn
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5f11608..ee83619 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -85,7 +85,7 @@ static void scsi_unprep_request(struct request *req)
 {
 	struct scsi_cmnd *cmd = req->special;
 
-	req->cmd_flags &= ~REQ_DONTPREP;
+	blk_unprep_request(req);
 	req->special = NULL;
 
 	scsi_put_command(cmd);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 204fbe2..6bba04c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -200,6 +200,7 @@ struct request_pm_state
 typedef void (request_fn_proc) (struct request_queue *q);
 typedef int (make_request_fn) (struct request_queue *q, struct bio *bio);
 typedef int (prep_rq_fn) (struct request_queue *, struct request *);
+typedef void (unprep_rq_fn) (struct request_queue *, struct request *);
 typedef void (unplug_fn) (struct request_queue *);
 
 struct bio_vec;
@@ -282,6 +283,7 @@ struct request_queue
 	request_fn_proc		*request_fn;
 	make_request_fn		*make_request_fn;
 	prep_rq_fn		*prep_rq_fn;
+	unprep_rq_fn		*unprep_rq_fn;
 	unplug_fn		*unplug_fn;
 	merge_bvec_fn		*merge_bvec_fn;
 	prepare_flush_fn	*prepare_flush_fn;
@@ -841,6 +843,7 @@ extern void blk_complete_request(struct request *);
 extern void __blk_complete_request(struct request *);
 extern void blk_abort_request(struct request *);
 extern void blk_abort_queue(struct request_queue *);
+extern void blk_unprep_request(struct request *);
 
 /*
  * Access functions for manipulating queue properties
@@ -885,6 +888,7 @@ extern int blk_queue_dma_drain(struct request_queue *q,
 extern void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn);
 extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
 extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
+extern void blk_queue_unprep_rq(struct request_queue *, unprep_rq_fn *ufn);
 extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
 extern void blk_queue_dma_alignment(struct request_queue *, int);
 extern void blk_queue_update_dma_alignment(struct request_queue *, int);
-- 
1.6.5

Re: [PATCH 1/3] block: implement an unprep function corresponding directly to prep

[James Bottomley]

On Thu, 2010-07-01 at 19:49 +0900, FUJITA Tomonori wrote:
> From: James Bottomley <James.Bottomley@suse.de>
> 
> Reviewed-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Well, I forgot this, but otherwise looks fine:

Signed-off-by: James Bottomley <James.Bottomley@suse.de>

> ---
>  block/blk-core.c        |   25 +++++++++++++++++++++++++
>  block/blk-settings.c    |   17 +++++++++++++++++
>  drivers/scsi/scsi_lib.c |    2 +-
>  include/linux/blkdev.h  |    4 ++++
>  4 files changed, 47 insertions(+), 1 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3c37894..5ab3ac2 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -608,6 +608,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
>  
>  	q->request_fn		= rfn;
>  	q->prep_rq_fn		= NULL;
> +	q->unprep_rq_fn		= NULL;
>  	q->unplug_fn		= generic_unplug_device;
>  	q->queue_flags		= QUEUE_FLAG_DEFAULT;
>  	q->queue_lock		= lock;
> @@ -2133,6 +2134,26 @@ static bool blk_update_bidi_request(struct request *rq, int error,
>  	return false;
>  }
>  
> +/**
> + * blk_unprep_request - unprepare a request
> + * @req:	the request
> + *
> + * This function makes a request ready for complete resubmission (or
> + * completion).  It happens only after all error handling is complete,
> + * so represents the appropriate moment to deallocate any resources
> + * that were allocated to the request in the prep_rq_fn.  The queue
> + * lock is held when calling this.
> + */
> +void blk_unprep_request(struct request *req)
> +{
> +	struct request_queue *q = req->q;
> +
> +	req->cmd_flags &= ~REQ_DONTPREP;
> +	if (q->unprep_rq_fn)
> +		q->unprep_rq_fn(q, req);
> +}
> +EXPORT_SYMBOL_GPL(blk_unprep_request);
> +
>  /*
>   * queue lock must be held
>   */
> @@ -2148,6 +2169,10 @@ static void blk_finish_request(struct request *req, int error)
>  
>  	blk_delete_timer(req);
>  
> +	if (req->cmd_flags & REQ_DONTPREP)
> +		blk_unprep_request(req);
> +
> +
>  	blk_account_io_done(req);
>  
>  	if (req->end_io)
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index f5ed5a1..a234f4b 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -37,6 +37,23 @@ void blk_queue_prep_rq(struct request_queue *q, prep_rq_fn *pfn)
>  EXPORT_SYMBOL(blk_queue_prep_rq);
>  
>  /**
> + * blk_queue_unprep_rq - set an unprepare_request function for queue
> + * @q:		queue
> + * @ufn:	unprepare_request function
> + *
> + * It's possible for a queue to register an unprepare_request callback
> + * which is invoked before the request is finally completed. The goal
> + * of the function is to deallocate any data that was allocated in the
> + * prepare_request callback.
> + *
> + */
> +void blk_queue_unprep_rq(struct request_queue *q, unprep_rq_fn *ufn)
> +{
> +	q->unprep_rq_fn = ufn;
> +}
> +EXPORT_SYMBOL(blk_queue_unprep_rq);
> +
> +/**
>   * blk_queue_merge_bvec - set a merge_bvec function for queue
>   * @q:		queue
>   * @mbfn:	merge_bvec_fn
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5f11608..ee83619 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -85,7 +85,7 @@ static void scsi_unprep_request(struct request *req)
>  {
>  	struct scsi_cmnd *cmd = req->special;
>  
> -	req->cmd_flags &= ~REQ_DONTPREP;
> +	blk_unprep_request(req);
>  	req->special = NULL;
>  
>  	scsi_put_command(cmd);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 204fbe2..6bba04c 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -200,6 +200,7 @@ struct request_pm_state
>  typedef void (request_fn_proc) (struct request_queue *q);
>  typedef int (make_request_fn) (struct request_queue *q, struct bio *bio);
>  typedef int (prep_rq_fn) (struct request_queue *, struct request *);
> +typedef void (unprep_rq_fn) (struct request_queue *, struct request *);
>  typedef void (unplug_fn) (struct request_queue *);
>  
>  struct bio_vec;
> @@ -282,6 +283,7 @@ struct request_queue
>  	request_fn_proc		*request_fn;
>  	make_request_fn		*make_request_fn;
>  	prep_rq_fn		*prep_rq_fn;
> +	unprep_rq_fn		*unprep_rq_fn;
>  	unplug_fn		*unplug_fn;
>  	merge_bvec_fn		*merge_bvec_fn;
>  	prepare_flush_fn	*prepare_flush_fn;
> @@ -841,6 +843,7 @@ extern void blk_complete_request(struct request *);
>  extern void __blk_complete_request(struct request *);
>  extern void blk_abort_request(struct request *);
>  extern void blk_abort_queue(struct request_queue *);
> +extern void blk_unprep_request(struct request *);
>  
>  /*
>   * Access functions for manipulating queue properties
> @@ -885,6 +888,7 @@ extern int blk_queue_dma_drain(struct request_queue *q,
>  extern void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn);
>  extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
>  extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
> +extern void blk_queue_unprep_rq(struct request_queue *, unprep_rq_fn *ufn);
>  extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
>  extern void blk_queue_dma_alignment(struct request_queue *, int);
>  extern void blk_queue_update_dma_alignment(struct request_queue *, int);

[PATCH 2/3] scsi: add sd_unprep_fn to free discard page

[FUJITA Tomonori]

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and
scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in
sd_prepare_discard().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/sd.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 86da819..2d4e3a8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -425,6 +425,7 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	sector_t sector = bio->bi_sector;
 	unsigned int nr_sectors = bio_sectors(bio);
 	unsigned int len;
+	int ret;
 	struct page *page;
 
 	if (sdkp->device->sector_size == 4096) {
@@ -465,7 +466,15 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	}
 
 	blk_add_request_payload(rq, page, len);
-	return scsi_setup_blk_pc_cmnd(sdp, rq);
+	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
+	rq->buffer = page_address(page);
+	return ret;
+}
+
+static void sd_unprep_fn(struct request_queue *q, struct request *rq)
+{
+	if (rq->cmd_flags & REQ_DISCARD)
+		__free_page(virt_to_page(rq->buffer));
 }
 
 /**
@@ -2242,6 +2251,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	sd_revalidate_disk(gd);
 
 	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
+	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
 
 	gd->driverfs_dev = &sdp->sdev_gendev;
 	gd->flags = GENHD_FL_EXT_DEVT;
-- 
1.6.5

[PATCH] scsi: address leak in the error path of discard page allocation

[Mike Snitzer]

On Thu, Jul 01 2010 at  6:49am -0400,
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> This fixes discard page leak by using q->unprep_rq_fn facility.
> 
> q->unprep_rq_fn is called when all the data buffer (req->bio and
> scsi_data_buffer) in the request is freed.
> 
> sd_unprep() uses rq->buffer to free discard page allocated in
> sd_prepare_discard().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more
elegant way to address this.

An alternative would be to check if the page is already allocated
(before allocating the page in scsi_setup_discard_cmnd)?

Please advise, thanks.
Mike



From: Mike Snitzer <snitzer@redhat.com>
Subject: scsi: address leak in the error path of discard page allocation

Be sure to free the discard page if scsi_setup_blk_pc_cmnd fails.
E.g. Returning BLKPREP_DEFER from scsi_setup_blk_pc_cmnd will not cause
the request to be processed by sd_unprep_fn before the request is
retried (preparation included).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/sd.c      |    6 +++++-
 include/linux/blkdev.h |    1 +
 3 files changed, 29 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/scsi/sd.c
===================================================================
--- linux-2.6.orig/drivers/scsi/sd.c
+++ linux-2.6/drivers/scsi/sd.c
@@ -466,7 +466,11 @@ static int scsi_setup_discard_cmnd(struc
 
 	blk_add_request_payload(rq, page, len);
 	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-	rq->buffer = page_address(page);
+	if (ret != BLKPREP_OK) {
+		blk_clear_request_payload(rq);
+		__free_page(page);
+	} else
+		rq->buffer = page_address(page);
 	return ret;
 }
 
Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c
+++ linux-2.6/block/blk-core.c
@@ -1164,6 +1164,29 @@ void blk_add_request_payload(struct requ
 }
 EXPORT_SYMBOL_GPL(blk_add_request_payload);
 
+/**
+ * blk_clear_request_payload - clear a request's payload
+ * @rq: request to update
+ *
+ * The driver needs to take care of freeing the payload itself.
+ */
+void blk_clear_request_payload(struct request *rq)
+{
+	struct bio *bio = rq->bio;
+
+	rq->__data_len = rq->resid_len = 0;
+	rq->nr_phys_segments = 0;
+	rq->buffer = NULL;
+
+	bio->bi_size = 0;
+	bio->bi_vcnt = 0;
+	bio->bi_phys_segments = 0;
+
+	bio->bi_io_vec->bv_page = NULL;
+	bio->bi_io_vec->bv_len = 0;
+}
+EXPORT_SYMBOL_GPL(blk_clear_request_payload);
+
 void init_request_from_bio(struct request *req, struct bio *bio)
 {
 	req->cpu = bio->bi_comp_cpu;
Index: linux-2.6/include/linux/blkdev.h
===================================================================
--- linux-2.6.orig/include/linux/blkdev.h
+++ linux-2.6/include/linux/blkdev.h
@@ -781,6 +781,7 @@ extern void blk_insert_request(struct re
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		unsigned int len);
+extern void blk_clear_request_payload(struct request *rq);
 extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
 extern int blk_lld_busy(struct request_queue *q);
 extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src,

Re: scsi: address leak in the error path of discard page allocation

[Mike Snitzer]

On Thu, Jul 01 2010 at  9:03am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Jul 01 2010 at  6:49am -0400,
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > This fixes discard page leak by using q->unprep_rq_fn facility.
> > 
> > q->unprep_rq_fn is called when all the data buffer (req->bio and
> > scsi_data_buffer) in the request is freed.
> > 
> > sd_unprep() uses rq->buffer to free discard page allocated in
> > sd_prepare_discard().
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Thanks for sorting this out Tomo, all 3 patches work great!
> 
> BTW, there is one remaining (rare) leak in the allocation path.
> 
> The following patch serves to fix it but I'm not sure if there is a more
> elegant way to address this.

I've continued to look at this to arrive at alternative implementation.
Here is a summary of the problem:

A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause
a discard request to get completely stripped down ('blk_finish_request'
isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by
'scsi_prep_return' for none BLKPREP_OK return).  Therefore the discard
request's page will _not_ get cleaned up.

Aside from code inspection, I confirmed this by adding some test code to
force a one-time initial BLKPREP_DEFER return from
'scsi_setup_discard_cmnd'.

> An alternative would be to check if the page is already allocated
> (before allocating the page in scsi_setup_discard_cmnd)?

Unfortunatey this "alternative" won't work because it completely ignores
the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.
 
> Please advise, thanks.

In short, I'm not too happy that the following patch doesn't allow for
centralized cleanup of the discard request's page (via sd_unprep_fn).
But in order to do that we'd likely have to:
1) relax blk_finish_request's REQ_DONTPREP constraint
2) add other weird conditionals within blk_unprep_request because
   the discard request wasn't _really_ prepared?

So given this I'm inclined to stick with the following patch.

Jens and/or James, what do you think?

Mike

> From: Mike Snitzer <snitzer@redhat.com>
> Subject: scsi: address leak in the error path of discard page allocation
> 
> Be sure to free the discard page if scsi_setup_blk_pc_cmnd fails.
> E.g. Returning BLKPREP_DEFER from scsi_setup_blk_pc_cmnd will not cause
> the request to be processed by sd_unprep_fn before the request is
> retried (preparation included).
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> 
> ---
>  block/blk-core.c       |   23 +++++++++++++++++++++++
>  drivers/scsi/sd.c      |    6 +++++-
>  include/linux/blkdev.h |    1 +
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/scsi/sd.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/sd.c
> +++ linux-2.6/drivers/scsi/sd.c
> @@ -466,7 +466,11 @@ static int scsi_setup_discard_cmnd(struc
>  
>  	blk_add_request_payload(rq, page, len);
>  	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> -	rq->buffer = page_address(page);
> +	if (ret != BLKPREP_OK) {
> +		blk_clear_request_payload(rq);
> +		__free_page(page);
> +	} else
> +		rq->buffer = page_address(page);
>  	return ret;
>  }
>  
> Index: linux-2.6/block/blk-core.c
> ===================================================================
> --- linux-2.6.orig/block/blk-core.c
> +++ linux-2.6/block/blk-core.c
> @@ -1164,6 +1164,29 @@ void blk_add_request_payload(struct requ
>  }
>  EXPORT_SYMBOL_GPL(blk_add_request_payload);
>  
> +/**
> + * blk_clear_request_payload - clear a request's payload
> + * @rq: request to update
> + *
> + * The driver needs to take care of freeing the payload itself.
> + */
> +void blk_clear_request_payload(struct request *rq)
> +{
> +	struct bio *bio = rq->bio;
> +
> +	rq->__data_len = rq->resid_len = 0;
> +	rq->nr_phys_segments = 0;
> +	rq->buffer = NULL;
> +
> +	bio->bi_size = 0;
> +	bio->bi_vcnt = 0;
> +	bio->bi_phys_segments = 0;
> +
> +	bio->bi_io_vec->bv_page = NULL;
> +	bio->bi_io_vec->bv_len = 0;
> +}
> +EXPORT_SYMBOL_GPL(blk_clear_request_payload);
> +
>  void init_request_from_bio(struct request *req, struct bio *bio)
>  {
>  	req->cpu = bio->bi_comp_cpu;
> Index: linux-2.6/include/linux/blkdev.h
> ===================================================================
> --- linux-2.6.orig/include/linux/blkdev.h
> +++ linux-2.6/include/linux/blkdev.h
> @@ -781,6 +781,7 @@ extern void blk_insert_request(struct re
>  extern void blk_requeue_request(struct request_queue *, struct request *);
>  extern void blk_add_request_payload(struct request *rq, struct page *page,
>  		unsigned int len);
> +extern void blk_clear_request_payload(struct request *rq);
>  extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
>  extern int blk_lld_busy(struct request_queue *q);
>  extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src,

Re: scsi: address leak in the error path of discard page allocation

[James Bottomley]

On Thu, 2010-07-01 at 16:15 -0400, Mike Snitzer wrote:
> On Thu, Jul 01 2010 at  9:03am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > On Thu, Jul 01 2010 at  6:49am -0400,
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > This fixes discard page leak by using q->unprep_rq_fn facility.
> > > 
> > > q->unprep_rq_fn is called when all the data buffer (req->bio and
> > > scsi_data_buffer) in the request is freed.
> > > 
> > > sd_unprep() uses rq->buffer to free discard page allocated in
> > > sd_prepare_discard().
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > 
> > Thanks for sorting this out Tomo, all 3 patches work great!
> > 
> > BTW, there is one remaining (rare) leak in the allocation path.
> > 
> > The following patch serves to fix it but I'm not sure if there is a more
> > elegant way to address this.
> 
> I've continued to look at this to arrive at alternative implementation.
> Here is a summary of the problem:
> 
> A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause
> a discard request to get completely stripped down ('blk_finish_request'
> isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by
> 'scsi_prep_return' for none BLKPREP_OK return).  Therefore the discard
> request's page will _not_ get cleaned up.
> 
> Aside from code inspection, I confirmed this by adding some test code to
> force a one-time initial BLKPREP_DEFER return from
> 'scsi_setup_discard_cmnd'.
> 
> > An alternative would be to check if the page is already allocated
> > (before allocating the page in scsi_setup_discard_cmnd)?
> 
> Unfortunatey this "alternative" won't work because it completely ignores
> the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.
>  
> > Please advise, thanks.
> 
> In short, I'm not too happy that the following patch doesn't allow for
> centralized cleanup of the discard request's page (via sd_unprep_fn).
> But in order to do that we'd likely have to:
> 1) relax blk_finish_request's REQ_DONTPREP constraint
> 2) add other weird conditionals within blk_unprep_request because
>    the discard request wasn't _really_ prepared?
> 
> So given this I'm inclined to stick with the following patch.
> 
> Jens and/or James, what do you think?

The rules are pretty clear:  Unprep is only called if the request gets
prepped ... that means you have to return BLKPREP_OK.  Defer or kill
assume there's no teardown to do, so the allocation (if it took place)
must be reversed before returning them

James

Re: scsi: address leak in the error path of discard page allocation

[Mike Snitzer]

On Thu, Jul 01 2010 at  4:19pm -0400,
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-07-01 at 16:15 -0400, Mike Snitzer wrote:
> > On Thu, Jul 01 2010 at  9:03am -0400,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > On Thu, Jul 01 2010 at  6:49am -0400,
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > This fixes discard page leak by using q->unprep_rq_fn facility.
> > > > 
> > > > q->unprep_rq_fn is called when all the data buffer (req->bio and
> > > > scsi_data_buffer) in the request is freed.
> > > > 
> > > > sd_unprep() uses rq->buffer to free discard page allocated in
> > > > sd_prepare_discard().
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > 
> > > Thanks for sorting this out Tomo, all 3 patches work great!
> > > 
> > > BTW, there is one remaining (rare) leak in the allocation path.
> > > 
> > > The following patch serves to fix it but I'm not sure if there is a more
> > > elegant way to address this.
> > 
> > I've continued to look at this to arrive at alternative implementation.
> > Here is a summary of the problem:
> > 
> > A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause
> > a discard request to get completely stripped down ('blk_finish_request'
> > isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by
> > 'scsi_prep_return' for none BLKPREP_OK return).  Therefore the discard
> > request's page will _not_ get cleaned up.
> > 
> > Aside from code inspection, I confirmed this by adding some test code to
> > force a one-time initial BLKPREP_DEFER return from
> > 'scsi_setup_discard_cmnd'.
> > 
> > > An alternative would be to check if the page is already allocated
> > > (before allocating the page in scsi_setup_discard_cmnd)?
> > 
> > Unfortunatey this "alternative" won't work because it completely ignores
> > the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.
> >  
> > > Please advise, thanks.
> > 
> > In short, I'm not too happy that the following patch doesn't allow for
> > centralized cleanup of the discard request's page (via sd_unprep_fn).
> > But in order to do that we'd likely have to:
> > 1) relax blk_finish_request's REQ_DONTPREP constraint
> > 2) add other weird conditionals within blk_unprep_request because
> >    the discard request wasn't _really_ prepared?
> > 
> > So given this I'm inclined to stick with the following patch.
> > 
> > Jens and/or James, what do you think?
> 
> The rules are pretty clear:  Unprep is only called if the request gets
> prepped ... that means you have to return BLKPREP_OK.  Defer or kill
> assume there's no teardown to do, so the allocation (if it took place)
> must be reversed before returning them

OK, thanks for clarifying.  This confirms that the general approach I
took in this patch is correct.  It remains to be seen if Jens is
agreeable with blk_clear_request_payload.

I know Christoph thought my introduction and use of
blk_clear_request_payload was reasonable.  Christoph, please feel free
to add your Ack to this patch if you approve.

I look forward to feedback from Tomo and Jens now too.  Hopefully Jens
will pick this patch up.

regards,
Mike

Re: scsi: address leak in the error path of discard page allocation

[Christoph Hellwig]

On Thu, Jul 01, 2010 at 05:07:49PM -0400, Mike Snitzer wrote:
> OK, thanks for clarifying.  This confirms that the general approach I
> took in this patch is correct.  It remains to be seen if Jens is
> agreeable with blk_clear_request_payload.
> 
> I know Christoph thought my introduction and use of
> blk_clear_request_payload was reasonable.  Christoph, please feel free
> to add your Ack to this patch if you approve.

Yes, I think that's the right way to do it.


Btw, guys - what happened to the idea of trimming useless context in
mail replies?  This thread got almost unreadable because of the quoting
at this point.

Re: scsi: address leak in the error path of discard page allocation

[FUJITA Tomonori]

On Thu, 01 Jul 2010 15:19:08 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-07-01 at 16:15 -0400, Mike Snitzer wrote:
> > On Thu, Jul 01 2010 at  9:03am -0400,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > On Thu, Jul 01 2010 at  6:49am -0400,
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > This fixes discard page leak by using q->unprep_rq_fn facility.
> > > > 
> > > > q->unprep_rq_fn is called when all the data buffer (req->bio and
> > > > scsi_data_buffer) in the request is freed.
> > > > 
> > > > sd_unprep() uses rq->buffer to free discard page allocated in
> > > > sd_prepare_discard().
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > 
> > > Thanks for sorting this out Tomo, all 3 patches work great!
> > > 
> > > BTW, there is one remaining (rare) leak in the allocation path.
> > > 
> > > The following patch serves to fix it but I'm not sure if there is a more
> > > elegant way to address this.
> > 
> > I've continued to look at this to arrive at alternative implementation.
> > Here is a summary of the problem:
> > 
> > A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause
> > a discard request to get completely stripped down ('blk_finish_request'
> > isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by
> > 'scsi_prep_return' for none BLKPREP_OK return).  Therefore the discard
> > request's page will _not_ get cleaned up.
> > 
> > Aside from code inspection, I confirmed this by adding some test code to
> > force a one-time initial BLKPREP_DEFER return from
> > 'scsi_setup_discard_cmnd'.
> > 
> > > An alternative would be to check if the page is already allocated
> > > (before allocating the page in scsi_setup_discard_cmnd)?
> > 
> > Unfortunatey this "alternative" won't work because it completely ignores
> > the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.
> >  
> > > Please advise, thanks.
> > 
> > In short, I'm not too happy that the following patch doesn't allow for
> > centralized cleanup of the discard request's page (via sd_unprep_fn).
> > But in order to do that we'd likely have to:
> > 1) relax blk_finish_request's REQ_DONTPREP constraint
> > 2) add other weird conditionals within blk_unprep_request because
> >    the discard request wasn't _really_ prepared?
> > 
> > So given this I'm inclined to stick with the following patch.
> > 
> > Jens and/or James, what do you think?
> 
> The rules are pretty clear:  Unprep is only called if the request gets
> prepped ... that means you have to return BLKPREP_OK.  Defer or kill
> assume there's no teardown to do, so the allocation (if it took place)
> must be reversed before returning them

Seems that scsi-ml calls scsi_unprep_request() for not-prepped
requests in scsi_init_io error path. So we could move that
scsi_unprep_request() to the error path in scsi_prep_return(). Then we
can free discard page in the single place.

Applying the rule strictly is fine by me too; we remove
scsi_unprep_request() in scsi_init_io error path and clean up things
in each prep function's error path.


Btw, blk_clear_request_payload() is necessary?

Making sure that a request is clean is not a bad idea but if we hit
BLKPREP_KILL or BLKPREP_DEFER, we call
blk_end_request(). blk_end_request() can free a request properly even
if we don't do something like blk_clear_request_payload?

Re: scsi: address leak in the error path of discard page allocation

[Christoph Hellwig]

On Fri, Jul 02, 2010 at 01:53:14PM +0900, FUJITA Tomonori wrote:
> Seems that scsi-ml calls scsi_unprep_request() for not-prepped
> requests in scsi_init_io error path. So we could move that
> scsi_unprep_request() to the error path in scsi_prep_return(). Then we
> can free discard page in the single place.
> 
> Applying the rule strictly is fine by me too; we remove
> scsi_unprep_request() in scsi_init_io error path and clean up things
> in each prep function's error path.

That would be my preference.  Making sure a function cleans up all
allocations / state changes on errors means code is a lot fragile and
easier to understand.

> Btw, blk_clear_request_payload() is necessary?
> 
> Making sure that a request is clean is not a bad idea but if we hit
> BLKPREP_KILL or BLKPREP_DEFER, we call
> blk_end_request(). blk_end_request() can free a request properly even
> if we don't do something like blk_clear_request_payload?

For BLKPREP_KILL we do call __blk_end_request_all, but for
BLKPREP_DEFER we don't.  In that case we just leave it on the queue for
a later retry.  So we either have to clean it up, or leave the detect
the case of a partially constructed command in ->prep_fn.  I think
cleaning up properly and having defined state when entering ->prep_fn is
the better variant.

Re: scsi: address leak in the error path of discard page allocation

[Mike Snitzer]

On Fri, Jul 02 2010 at  6:52am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> On Fri, Jul 02, 2010 at 01:53:14PM +0900, FUJITA Tomonori wrote:
> > Seems that scsi-ml calls scsi_unprep_request() for not-prepped
> > requests in scsi_init_io error path. So we could move that
> > scsi_unprep_request() to the error path in scsi_prep_return(). Then we
> > can free discard page in the single place.
> > 
> > Applying the rule strictly is fine by me too; we remove
> > scsi_unprep_request() in scsi_init_io error path and clean up things
> > in each prep function's error path.
> 
> That would be my preference.  Making sure a function cleans up all
> allocations / state changes on errors means code is a lot fragile and
> easier to understand.
> 
> > Btw, blk_clear_request_payload() is necessary?
> > 
> > Making sure that a request is clean is not a bad idea but if we hit
> > BLKPREP_KILL or BLKPREP_DEFER, we call
> > blk_end_request(). blk_end_request() can free a request properly even
> > if we don't do something like blk_clear_request_payload?
> 
> For BLKPREP_KILL we do call __blk_end_request_all, but for
> BLKPREP_DEFER we don't.  In that case we just leave it on the queue for
> a later retry.  So we either have to clean it up, or leave the detect
> the case of a partially constructed command in ->prep_fn.  I think
> cleaning up properly and having defined state when entering ->prep_fn is
> the better variant.

Right, I shared the same opinion earlier in this thread, so please let
your ACKs fly now that we seem to all be in agreement.

I'd like Jens to pick this patch up now-ish ;)

Mike

Re: scsi: address leak in the error path of discard page allocation

[FUJITA Tomonori]

On Fri, 2 Jul 2010 09:08:56 -0400
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Jul 02 2010 at  6:52am -0400,
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Fri, Jul 02, 2010 at 01:53:14PM +0900, FUJITA Tomonori wrote:
> > > Seems that scsi-ml calls scsi_unprep_request() for not-prepped
> > > requests in scsi_init_io error path. So we could move that
> > > scsi_unprep_request() to the error path in scsi_prep_return(). Then we
> > > can free discard page in the single place.
> > > 
> > > Applying the rule strictly is fine by me too; we remove
> > > scsi_unprep_request() in scsi_init_io error path and clean up things
> > > in each prep function's error path.
> > 
> > That would be my preference.  Making sure a function cleans up all
> > allocations / state changes on errors means code is a lot fragile and
> > easier to understand.
> > 
> > > Btw, blk_clear_request_payload() is necessary?
> > > 
> > > Making sure that a request is clean is not a bad idea but if we hit
> > > BLKPREP_KILL or BLKPREP_DEFER, we call
> > > blk_end_request(). blk_end_request() can free a request properly even
> > > if we don't do something like blk_clear_request_payload?
> > 
> > For BLKPREP_KILL we do call __blk_end_request_all, but for
> > BLKPREP_DEFER we don't.  In that case we just leave it on the queue for
> > a later retry.  So we either have to clean it up, or leave the detect
> > the case of a partially constructed command in ->prep_fn.  I think
> > cleaning up properly and having defined state when entering ->prep_fn is
> > the better variant.

I think that as long as we free an allocated page for discard,
scsi_setup_discard_cmnd or the block layer don't need to detect the
case of a partially constructed command.


> Right, I shared the same opinion earlier in this thread, so please let
> your ACKs fly now that we seem to all be in agreement.

As I wrote earlier, I think that we need to clean up (and fix) the
error handling of the prep path. Currently, it's just messy. Some
errors are handled in the prep functions while some are in
scsi_prep_return().

- we call scsi_put_command in two places (scsi_init_io and scsi_prep_return).

- scsi_init_io calls scsi_put_command directly for BLKPREP_KILL but
  calls it indirectly via scsi_unprep_request for BLKPREP_DEFER.

- If scsi_init_io() hits BLKPREP_KILL internally, we hit kernel oops
  in scsi_prep_return since we call scsi_put_command twice. (luckily,
  scsi_init_sgtable and scsi_alloc_sgtable in scsi_init_io return only
  DEFER).

you could add more if you like.

I think that handling all the errors in scsi_prep_return() is
cleaner. I'll send a patch.

If we prefer to make sure that a request is completely reset (as
blk_clear_request_payload does), then we can add it on the top of the
patch.

Re: [PATCH] scsi: address leak in the error path of discard page allocation

[Christoph Hellwig]

On Thu, Jul 01, 2010 at 09:03:28AM -0400, Mike Snitzer wrote:
> On Thu, Jul 01 2010 at  6:49am -0400,
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > This fixes discard page leak by using q->unprep_rq_fn facility.
> > 
> > q->unprep_rq_fn is called when all the data buffer (req->bio and
> > scsi_data_buffer) in the request is freed.
> > 
> > sd_unprep() uses rq->buffer to free discard page allocated in
> > sd_prepare_discard().
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Thanks for sorting this out Tomo, all 3 patches work great!
> 
> BTW, there is one remaining (rare) leak in the allocation path.
> 
> The following patch serves to fix it but I'm not sure if there is a more
> elegant way to address this.
> 
> An alternative would be to check if the page is already allocated
> (before allocating the page in scsi_setup_discard_cmnd)?

Ah, should have read your mail first, sorry..

Re: [PATCH 2/3] scsi: add sd_unprep_fn to free discard page

[Christoph Hellwig]

> sd_unprep() uses rq->buffer to free discard page allocated in
> sd_prepare_discard().

Eeek.  Accessing it using the bio in both haves seems a lot cleaner than
abusing this.  Especially as we don't really need a mapped page anyway
at least for WRITE SAME implementation.

> -	return scsi_setup_blk_pc_cmnd(sdp, rq);
> +	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> +	rq->buffer = page_address(page);
> +	return ret;

In addition I don't think this is quite correct.  You still need to undo
the payload addition manually if scsi_setup_blk_pc_cmnd fails, as we
haven't marked the request as REQ_DONTPREP at that point - it's only
done by scsi_prep_return for the BLKPREP_OK case.

Re: [PATCH 2/3] scsi: add sd_unprep_fn to free discard page

[Boaz Harrosh]

On 07/01/2010 01:49 PM, FUJITA Tomonori wrote:
> This fixes discard page leak by using q->unprep_rq_fn facility.
> 
> q->unprep_rq_fn is called when all the data buffer (req->bio and
> scsi_data_buffer) in the request is freed.
> 
> sd_unprep() uses rq->buffer to free discard page allocated in
> sd_prepare_discard().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Sorry for thinking about this just now. It was on the tip of my
mind, but I was too busy with other things to notice.

There is a simple more STD way to solve all this without the need
for any new API.

*Use the bio destructor to also deallocate it's pages*

Now that is not such a novel idea, it's used all over the place.
Well it's the proper way to submit a bio and be notified on done.
Since there are no bio leaks, we know all error paths are just
as covered.

For this to work all these patches could be removed and the
blk_add_request_payload need to change to recieve a bio destructor
pointer.

If you ask me I hate what was done with discard at the block layer
since it's only in Jens tree for-next I think it is good time to change
it. What is this alloc the bio in one place and add it's pages in another
place. Rrrrr

to quote from the author himself:
<block-core.c>
/**
 * blk_add_request_payload - add a payload to a request
 <snip>
 *
 * Note that this is a quite horrible hack and nothing but handling of
 * discard requests should ever use it.
 */
void blk_add_request_payload(struct request *rq, struct page *page,
		unsigned int len)
</block-core.c>

The block-layer users think of a discard as zero-load, so should the
block-layer. The block driver like sd that needs to add a payload can
set in a bio just like any other place. when allocating the bio, set
it's destructor and be done with it. All allocations and cleanups are
done at one place, at same level.

As it stands now blk_add_request_payload will have to either chain the
bio destructors and/or if it knows where it's allocated then chain to
the proper bio_free, after calling the block-driver supplied callback.

BTW where is that zero-pages discard bio allocated. Why is it not allocated
inside blk_add_request_payload? What is the meaning of zero length bio?
I though everywhere we check for "if (req->bio)" not "if (bio_size(req_bio))"
isn't all this asking for trouble?

my $0.017
Boaz

> ---
>  drivers/scsi/sd.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 86da819..2d4e3a8 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -425,6 +425,7 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>  	sector_t sector = bio->bi_sector;
>  	unsigned int nr_sectors = bio_sectors(bio);
>  	unsigned int len;
> +	int ret;
>  	struct page *page;
>  
>  	if (sdkp->device->sector_size == 4096) {
> @@ -465,7 +466,15 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>  	}
>  
>  	blk_add_request_payload(rq, page, len);
> -	return scsi_setup_blk_pc_cmnd(sdp, rq);
> +	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> +	rq->buffer = page_address(page);
> +	return ret;
> +}
> +
> +static void sd_unprep_fn(struct request_queue *q, struct request *rq)
> +{
> +	if (rq->cmd_flags & REQ_DISCARD)
> +		__free_page(virt_to_page(rq->buffer));
>  }
>  
>  /**
> @@ -2242,6 +2251,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>  	sd_revalidate_disk(gd);
>  
>  	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
> +	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
>  
>  	gd->driverfs_dev = &sdp->sdev_gendev;
>  	gd->flags = GENHD_FL_EXT_DEVT;

[PATCH 3/3] scsi: remove unused free discard page in sd_done

[FUJITA Tomonori]

- sd_done isn't called for pc request so we never call the code.
- we use sd_unprep to free discard page now.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/sd.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2d4e3a8..aa6b48b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1179,15 +1179,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	int sense_valid = 0;
 	int sense_deferred = 0;
 
-	/*
-	 * If this is a discard request that originated from the kernel
-	 * we need to free our payload here.  Note that we need to check
-	 * the request flag as the normal payload rules apply for
-	 * pass-through UNMAP / WRITE SAME requests.
-	 */
-	if (SCpnt->request->cmd_flags & REQ_DISCARD)
-		__free_page(bio_page(SCpnt->request->bio));
-
 	if (result) {
 		sense_valid = scsi_command_normalize_sense(SCpnt, &sshdr);
 		if (sense_valid)
-- 
1.6.5

Re: [PATCH 3/3] scsi: remove unused free discard page in sd_done

[Christoph Hellwig]

On Thu, Jul 01, 2010 at 07:49:19PM +0900, FUJITA Tomonori wrote:
> - sd_done isn't called for pc request so we never call the code.
> - we use sd_unprep to free discard page now.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

Re:

[Jens Axboe]

On 2010-07-01 12:49, FUJITA Tomonori wrote:
> This patchset fixes page leak issue in discard commands with unprep
> facility that James posted:
> 
> http://marc.info/?l=linux-scsi&m=127791727508214&w=2
> 
> The 1/3 patch adds unprep facility to the block layer (identical to
> what James posted).
> 
> The 2/3 patch frees a page for discard commands by using the unprep
> facility. James' original patch doesn't work since it accesses to
> rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is
> called when all the data buffer (req->bio and scsi_data_buffer) in the
> request is freed.
> 
> I use rq->buffer to keep track of an allocated page as the block layer
> sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't
> use rq->buffer (rq->buffer is set to NULL). So I can't say that I like
> it lots. Any other way to do that?
> 
> The 3/3 path just removes the dead code.

I've queued up these three for 2.6.36.

-- 
Jens Axboe

Re: add sd_unprep_fn to free discard page

[Boaz Harrosh]

On 07/01/2010 01:49 PM, FUJITA Tomonori wrote:
> This patchset fixes page leak issue in discard commands with unprep
> facility that James posted:
> 
> http://marc.info/?l=linux-scsi&m=127791727508214&w=2
> 
> The 1/3 patch adds unprep facility to the block layer (identical to
> what James posted).
> 

Alternatively to this patch you could also call scsi_driver->done()
on all commands. There are only two users (sd sr) it's not that bad to add
an if (blk_pc_req()) inside these ->done() function.

> The 2/3 patch frees a page for discard commands by using the unprep
> facility. James' original patch doesn't work since it accesses to
> rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is
> called when all the data buffer (req->bio and scsi_data_buffer) in the
> request is freed.
> 
> I use rq->buffer to keep track of an allocated page as the block layer
> sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't
> use rq->buffer (rq->buffer is set to NULL). So I can't say that I like
> it lots. Any other way to do that?
> 

rq->buffer is intended for block-driver use as well as req->special.
sd+scsi-ml is the block-driver here. req->special is used by scsi-ml
and rq->buffer is set to NULL inside the call to
scsi_setup_blk_pc_cmnd/scsi_setup_fs_cmnd. Since you set the ->buffer
after the call to scsi_setup_blk_pc_cmnd you should be in the clear.

I think scsi-ml should stop setting rq->buffer to NULL and leave it
be for ULD use. It is left from the time that LLDs where converted
to use BIOs, just to make sure out-of-tree drivers crash.

Boaz
> The 3/3 path just removes the dead code.
> 
> This is against Jens' for-2.6.36.
> 
> The git tree is also available:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git unprep
> 
> I'll update the discard FS request conversion on the top of this soon. But this can be applied independently (and fixes the memory leak).
> 
> =
>  block/blk-core.c        |   25 +++++++++++++++++++++++++
>  block/blk-settings.c    |   17 +++++++++++++++++
>  drivers/scsi/scsi_lib.c |    2 +-
>  drivers/scsi/sd.c       |   25 +++++++++++++++----------
>  include/linux/blkdev.h  |    4 ++++
>  5 files changed, 62 insertions(+), 11 deletions(-)
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: add sd_unprep_fn to free discard page

[James Bottomley]

On Thu, 2010-07-01 at 16:40 +0300, Boaz Harrosh wrote:
> On 07/01/2010 01:49 PM, FUJITA Tomonori wrote:
> > This patchset fixes page leak issue in discard commands with unprep
> > facility that James posted:
> > 
> > http://marc.info/?l=linux-scsi&m=127791727508214&w=2
> > 
> > The 1/3 patch adds unprep facility to the block layer (identical to
> > what James posted).
> > 
> 
> Alternatively to this patch you could also call scsi_driver->done()
> on all commands. There are only two users (sd sr) it's not that bad to add
> an if (blk_pc_req()) inside these ->done() function.

We could, but it wouldn't fix the problem.  The system issue is that
->done isn't symmetrical to ->prep_rq_fn() ... ->done() sits in the
middle of the SCSI completion path and we could decide to requeue the
command after calling it (that's why we can't free the discard page
either in ->done() or in the midlayer where ->done() is called).

The system solution is a symmetrical pair for ->prep_rq_fn() which is
only called either after all error handling is complete, or we decide to
completely strip the command down for a retry that goes through prep
again.

> > The 2/3 patch frees a page for discard commands by using the unprep
> > facility. James' original patch doesn't work since it accesses to
> > rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is
> > called when all the data buffer (req->bio and scsi_data_buffer) in the
> > request is freed.
> > 
> > I use rq->buffer to keep track of an allocated page as the block layer
> > sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't
> > use rq->buffer (rq->buffer is set to NULL). So I can't say that I like
> > it lots. Any other way to do that?
> > 
> 
> rq->buffer is intended for block-driver use as well as req->special.
> sd+scsi-ml is the block-driver here. req->special is used by scsi-ml
> and rq->buffer is set to NULL inside the call to
> scsi_setup_blk_pc_cmnd/scsi_setup_fs_cmnd. Since you set the ->buffer
> after the call to scsi_setup_blk_pc_cmnd you should be in the clear.
> 
> I think scsi-ml should stop setting rq->buffer to NULL and leave it
> be for ULD use. It is left from the time that LLDs where converted
> to use BIOs, just to make sure out-of-tree drivers crash.

So I buy this more.  There is a slight assymmetry in that we have the
bio in prep, but not in unprep.  Since requests can complete partially
(freeing the bios on the way), there's no real way to avoid this.  I
think the use of ->buffer is a bit unsavoury but it looks acceptable.

James

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bao D. Nguyen]

On 8/22/2024 11:13 AM, Bart Van Assche wrote:
> On 8/21/24 6:05 PM, Bao D. Nguyen wrote:
>> If I understand correctly, the link is hibernated because we had a 
>> successful ufshcd_uic_hibern8_enter() earlier. Then the 
>> ufshcd_uic_pwr_ctrl() is invoked probably from a power mode change 
>> command? (a callstack would be helpful in this case).
> 
> Hi Bao,
> 
> ufshcd_uic_hibern8_enter() calls ufshcd_uic_pwr_ctrl() directly. The
> former function causes the latter to send the UIC_CMD_DME_HIBER_ENTER
> command. Although UIC_CMD_DME_HIBER_ENTER causes the link to enter the
> hibernation state, the code in ufshcd_uic_pwr_ctrl() for re-enabling
> interrupts causes the UFS host controller that I'm testing to exit the
> hibernation state.
> 
>> Anyway, accessing the UFSHCI host controller registers space somehow 
>> affected the M-PHY link state? If my understanding is correct, it 
>> seems like a hardware issue that we are trying to work around?
> 
> Yes, this is a workaround particular behavior of a particular UFS
> controller.

Thank you for the clarification, Bart.
I am just curious about providing workaround for a hardware issue in the 
ufs core driver. Sometimes I notice the community is not accepting such 
a change and push the change to be implemented in the vendor/platform 
drivers.

Now about your workaround, I have the same concern as Bean.
For a ufshcd_uic_pwr_ctrl() command, i.e PMC, hibern8_enter/exit() 
commands, you will get 2 ufshcd_uic_cmd_compl() interrupts or 1 uic 
completion interrupt with 2 status bits set at once.
a. UIC_COMMAND_COMPL is set
b. and one of these bits UIC_POWER_MODE || UIC_HIBERNATE_EXIT || 
UIC_HIBERNATE_ENTER is set, depending on the completed uic command. 


In your proposed change to ufshcd_uic_cmd_compl(), you are signalling 
both complete(&cmd->done) and complete(hba->uic_async_done) for a single 
ufshcd_uic_pwr_ctrl() command which can cause issues.

Thanks, Bao

> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche]

On 8/22/24 1:54 PM, Bao D. Nguyen wrote:
> I am just curious about providing workaround for a hardware issue in the 
> ufs core driver. Sometimes I notice the community is not accepting such 
> a change and push the change to be implemented in the vendor/platform 
> drivers.

There are two reasons why I propose to implement this workaround as a
change for the UFS driver core:
- I am not aware of any way to implement the behavior change in a vendor
   driver without modifying the UFS driver core.
- The workaround results in a simplification of the UFS driver core
   code. More lines are removed from the UFS driver than added.

Although it would be possible to select whether the old or the new
behavior is selected by introducing yet another host controller quirk, I
prefer not to do that because it would make the UFSHCI driver even more
complex.

> In your proposed change to ufshcd_uic_cmd_compl(), you are signalling 
> both complete(&cmd->done) and complete(hba->uic_async_done) for a single 
> ufshcd_uic_pwr_ctrl() command which can cause issues.

Please take another look at patch 2/2. With or without this patch
applied, only hba->uic_async_done is signaled when a power mode UIC
command completes.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:08:24PM -0700, Bart Van Assche wrote:
> On 8/22/24 1:54 PM, Bao D. Nguyen wrote:
> > I am just curious about providing workaround for a hardware issue in the
> > ufs core driver. Sometimes I notice the community is not accepting such
> > a change and push the change to be implemented in the vendor/platform
> > drivers.
> 
> There are two reasons why I propose to implement this workaround as a
> change for the UFS driver core:
> - I am not aware of any way to implement the behavior change in a vendor
>   driver without modifying the UFS driver core.

Unfortunately you never mentioned which UFS controller this behavior applies to.

> - The workaround results in a simplification of the UFS driver core
>   code. More lines are removed from the UFS driver than added.
> 

This doesn't justify the modification of the UFS code driver for an errantic
behavior of a UFS controller.

> Although it would be possible to select whether the old or the new
> behavior is selected by introducing yet another host controller quirk, I
> prefer not to do that because it would make the UFSHCI driver even more
> complex.
> 

I strongly believe that using the quirk is the way forward to address this
issue. Because this is not a documented behavior to be handled in the core
driver and also defeats the purpose of having the quirks in first place.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche]

On 8/23/24 5:01 AM, Manivannan Sadhasivam wrote:
> Unfortunately you never mentioned which UFS controller this behavior applies to.

Does your employer allow you to publish detailed information about
unreleased SoCs?

>> - The workaround results in a simplification of the UFS driver core
>>    code. More lines are removed from the UFS driver than added.
> 
> This doesn't justify the modification of the UFS code driver for an errantic
> behavior of a UFS controller.

Patch 2/2 deletes more code than it adds and hence helps to make the UFS
driver core easier to maintain. Adding yet another quirk would make the
UFS driver core more complicated and hence harder to maintain.

>> Although it would be possible to select whether the old or the new
>> behavior is selected by introducing yet another host controller quirk, I
>> prefer not to do that because it would make the UFSHCI driver even more
>> complex.
> 
> I strongly believe that using the quirk is the way forward to address this
> issue. Because this is not a documented behavior to be handled in the core
> driver and also defeats the purpose of having the quirks in first place.

Adding a quirk is not an option in this case because adding a new quirk
without code that uses the quirk is not allowed. It may take another
year before the code that uses that new quirk is sent upstream because
the team that sends Pixel SoC drivers upstream only does this after
devices with that SoC are publicly available.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 07:23:57AM -0700, Bart Van Assche wrote:
> On 8/23/24 5:01 AM, Manivannan Sadhasivam wrote:
> > Unfortunately you never mentioned which UFS controller this behavior applies to.
> 
> Does your employer allow you to publish detailed information about
> unreleased SoCs?
> 

Certainly not! But in that case I will explicitly mention that the controller is
used in an upcoming SoC or something like that. Otherwise, how can the community
know whether you are sending the patch for an existing controller or a secret
one?

> > > - The workaround results in a simplification of the UFS driver core
> > >    code. More lines are removed from the UFS driver than added.
> > 
> > This doesn't justify the modification of the UFS code driver for an errantic
> > behavior of a UFS controller.
> 
> Patch 2/2 deletes more code than it adds and hence helps to make the UFS
> driver core easier to maintain. Adding yet another quirk would make the
> UFS driver core more complicated and hence harder to maintain.
> 

IMO nobody can make the UFS driver more complicated. It is complicated at its
best :)

But you are just applying the plaster in the core code for a quirky behavior of
an unreleased SoC. IMO that is not something we would want. Imagine if other
vendor also decides to do the same with the argument of removing more lines of
code etc... we will end up with a situation where the core code doing something
out of the spec for a buggy controller without any mentioning of the quirky
behavior.

So that will make the code more complex to understand.

> > > Although it would be possible to select whether the old or the new
> > > behavior is selected by introducing yet another host controller quirk, I
> > > prefer not to do that because it would make the UFSHCI driver even more
> > > complex.
> > 
> > I strongly believe that using the quirk is the way forward to address this
> > issue. Because this is not a documented behavior to be handled in the core
> > driver and also defeats the purpose of having the quirks in first place.
> 
> Adding a quirk is not an option in this case because adding a new quirk
> without code that uses the quirk is not allowed. It may take another
> year before the code that uses that new quirk is sent upstream because
> the team that sends Pixel SoC drivers upstream only does this after
> devices with that SoC are publicly available.
> 

Then why can't you send the change at that time? We do the same for Qcom SoCs
all the time and I'm pretty sure that the Pixel SoCs are no different.

At that time, the SoC may get a new revision and we may end up not needing the
quirk at all. I'm not saying that it will happen, but that is a common practice
among the semiconductor companies.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche]

On 8/23/24 7:58 AM, Manivannan Sadhasivam wrote:
> Then why can't you send the change at that time?

We use the Android GKI kernel and any patches must be sent upstream
first before these can be considered for inclusion in the GKI kernel.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 09:07:18AM -0700, Bart Van Assche wrote:
> On 8/23/24 7:58 AM, Manivannan Sadhasivam wrote:
> > Then why can't you send the change at that time?
> 
> We use the Android GKI kernel and any patches must be sent upstream
> first before these can be considered for inclusion in the GKI kernel.
> 

But that's the same requirement for other SoC vendors as well. Anyway, these
don't justify the fact that the core code should be modified to workaround a
controller defect. Please use quirks as like other vendors.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche]

On 8/23/24 9:48 AM, Manivannan Sadhasivam wrote:
> On Fri, Aug 23, 2024 at 09:07:18AM -0700, Bart Van Assche wrote:
>> On 8/23/24 7:58 AM, Manivannan Sadhasivam wrote:
>>> Then why can't you send the change at that time?
>>
>> We use the Android GKI kernel and any patches must be sent upstream
>> first before these can be considered for inclusion in the GKI kernel.
> 
> But that's the same requirement for other SoC vendors as well. Anyway, these
> don't justify the fact that the core code should be modified to workaround a
> controller defect. Please use quirks as like other vendors.

Let me repeat what I mentioned earlier:
* Introducing a new quirk without introducing a user for that quirk is
   not acceptable because that would involve introducing code that is
   dead code from the point of view of the upstream kernel.
* The UFS driver core is already complicated. If we don't need a new
   quirk we shouldn't introduce a new quirk.

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 11:05:12AM -0700, Bart Van Assche wrote:
> On 8/23/24 9:48 AM, Manivannan Sadhasivam wrote:
> > On Fri, Aug 23, 2024 at 09:07:18AM -0700, Bart Van Assche wrote:
> > > On 8/23/24 7:58 AM, Manivannan Sadhasivam wrote:
> > > > Then why can't you send the change at that time?
> > > 
> > > We use the Android GKI kernel and any patches must be sent upstream
> > > first before these can be considered for inclusion in the GKI kernel.
> > 
> > But that's the same requirement for other SoC vendors as well. Anyway, these
> > don't justify the fact that the core code should be modified to workaround a
> > controller defect. Please use quirks as like other vendors.
> 
> Let me repeat what I mentioned earlier:
> * Introducing a new quirk without introducing a user for that quirk is
>   not acceptable because that would involve introducing code that is
>   dead code from the point of view of the upstream kernel.

As I pointed out earlier, you should just submit the quirk change when you are
submitting your driver. But you said that for GKI requirement you are doing the
change in core driver. But again, that is applicable for other vendors as well.
What if other vendors start adding the workaround in the core driver citing GKI
requirement (provided it also removes some code as you justified)? Will it be
acceptable? NO.

> * The UFS driver core is already complicated. If we don't need a new
>   quirk we shouldn't introduce a new quirk.
> 

Sorry, the quirk is a quirk. All the existing quirks can be worked around in the
core driver in some way. But we have the quirk mechanisms to specifically not to
do that to avoid polluting the core code which has to follow the spec.

Moreover, this workaround you are adding is not at all common for other
controllers. So this definitely doesn't justify modifying the core code.

IMO adding more code alone will not make a driver complicated, but changing the
logic will.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche]

On 8/23/24 7:29 PM, Manivannan Sadhasivam wrote:
> What if other vendors start adding the workaround in the core driver citing GKI
> requirement (provided it also removes some code as you justified)? Will it be
> acceptable? NO.

It's not up to you to define new rules for upstream kernel development.
Anyone is allowed to publish patches that rework kernel code, whether
or not the purpose of such a patch is to work around a SoC bug.

Additionally, it has already happened that one of your colleagues
submitted a workaround for a SoC bug to the UFS core driver.
 From the description of commit 0f52fcb99ea2 ("scsi: ufs: Try to save
power mode change and UIC cmd completion timeout"): "This is to deal
with the scenario in which completion has been raised but the one
waiting for the completion cannot be awaken in time due to kernel
scheduling problem." That description makes zero sense to me. My
conclusion from commit 0f52fcb99ea2 is that it is a workaround for a
bug in a UFS host controller, namely that a particular UFS host
controller not always generates a UIC completion interrupt when it
should.

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 07:48:50PM -0700, Bart Van Assche wrote:
> On 8/23/24 7:29 PM, Manivannan Sadhasivam wrote:
> > What if other vendors start adding the workaround in the core driver citing GKI
> > requirement (provided it also removes some code as you justified)? Will it be
> > acceptable? NO.
> 
> It's not up to you to define new rules for upstream kernel development.

I'm not framing new rules, but just pointing out the common practice.

> Anyone is allowed to publish patches that rework kernel code, whether
> or not the purpose of such a patch is to work around a SoC bug.
> 

Yes, at the same time if that code deviates from the norm, then anyone can
complain. We are all working towards making the code better.

> Additionally, it has already happened that one of your colleagues
> submitted a workaround for a SoC bug to the UFS core driver.
> From the description of commit 0f52fcb99ea2 ("scsi: ufs: Try to save
> power mode change and UIC cmd completion timeout"): "This is to deal
> with the scenario in which completion has been raised but the one
> waiting for the completion cannot be awaken in time due to kernel
> scheduling problem." That description makes zero sense to me. My
> conclusion from commit 0f52fcb99ea2 is that it is a workaround for a
> bug in a UFS host controller, namely that a particular UFS host
> controller not always generates a UIC completion interrupt when it
> should.
> 

0f52fcb99ea2 was submitted in 2020 before I started contributing to UFS driver
seriously. But the description of that commit never mentioned any issue with the
controller. It vaguely mentions 'kernel scheduling problem' which I don't know
how to interpret. If I were looking into the code at that time, I would've
definitely asked for clarity during the review phase.

But there is no need to take it as an example. I can only assert the fact that
working around the controller defect in core code when we already have quirks
for the same purpose defeats the purpose of quirks. And it will encourage other
people to start changing the core code in the future thus bypassing the quirks.

But I'm not a maintainer of this part of the code. So I cannot definitely stop
you from getting this patch merged. I'll leave it up to Martin to decide.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re:

[Can Guo]

On 1/1/1970 8:00 AM, wrote:
> On Fri, Aug 23, 2024 at 07:48:50PM -0700, Bart Van Assche wrote:
>> On 8/23/24 7:29 PM, Manivannan Sadhasivam wrote:
>>> What if other vendors start adding the workaround in the core driver citing GKI
>>> requirement (provided it also removes some code as you justified)? Will it be
>>> acceptable? NO.
>> It's not up to you to define new rules for upstream kernel development.
> I'm not framing new rules, but just pointing out the common practice.
>
>> Anyone is allowed to publish patches that rework kernel code, whether
>> or not the purpose of such a patch is to work around a SoC bug.
>>
> Yes, at the same time if that code deviates from the norm, then anyone can
> complain. We are all working towards making the code better.
>
>> Additionally, it has already happened that one of your colleagues
>> submitted a workaround for a SoC bug to the UFS core driver.
>>  From the description of commit 0f52fcb99ea2 ("scsi: ufs: Try to save
>> power mode change and UIC cmd completion timeout"): "This is to deal
>> with the scenario in which completion has been raised but the one
>> waiting for the completion cannot be awaken in time due to kernel
>> scheduling problem." That description makes zero sense to me. My
>> conclusion from commit 0f52fcb99ea2 is that it is a workaround for a
>> bug in a UFS host controller, namely that a particular UFS host
>> controller not always generates a UIC completion interrupt when it
>> should.
>>
> 0f52fcb99ea2 was submitted in 2020 before I started contributing to UFS driver
> seriously. But the description of that commit never mentioned any issue with the
> controller. It vaguely mentions 'kernel scheduling problem' which I don't know
> how to interpret. If I were looking into the code at that time, I would've
> definitely asked for clarity during the review phase.

0f52fcb99ea2 is my commit, apologize for the confusion due to poor commit msg.
What we were trying to fix was not a SoC BUG. More background for this change:
from our customer side, we used to hit corner cases where the UIC command is
sent, UFS host controller generates the UIC command completion interrupt fine,
then UIC completion IRQ handler fires and calls the complete(), however the
completion timeout error still happens. In this case, UFS, UFS host and UFS
driver are the victims. And whatever could cause this scheduling problem should
be fixed properly by the right PoC, but we thought making UFS driver robust in
this spot would be good for all of the users who may face the similar issue,
hence the change.

Thanks,
Can Guo.

>
> But there is no need to take it as an example. I can only assert the fact that
> working around the controller defect in core code when we already have quirks
> for the same purpose defeats the purpose of quirks. And it will encourage other
> people to start changing the core code in the future thus bypassing the quirks.
>
> But I'm not a maintainer of this part of the code. So I cannot definitely stop
> you from getting this patch merged. I'll leave it up to Martin to decide.
>
> - Mani
>

(no subject)

[Denis Arefev]

Date: Mon, 21 Nov 2022 13:29:03 +0300
Subject: [PATCH] scsi:bfa: Eliminated buffer overflow

Buffer 'cmd->adapter_hwpath' of size 32 accessed at
bfad_bsg.c:101:103 can overflow, since its index 'i'
can have value 32 that is out of range.

Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/scsi/bfa/bfad_bsg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
index be8dfbe13e90..78615ffc62ef 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -98,9 +98,9 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad, void *cmd)
 
 	/* set adapter hw path */
 	strcpy(iocmd->adapter_hwpath, bfad->pci_name);
-	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
+	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32-2; i++)
 		;
-	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
+	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32-1; )
 		;
 	iocmd->adapter_hwpath[i] = '\0';
 	iocmd->status = BFA_STATUS_OK;
-- 
2.25.1

Re:

[Jason Yan]

You may need a real subject, not a subject text in the email.

type "git help send-email" if you don't know how to use it.

On 2022/11/21 19:11, Denis Arefev wrote:
> Date: Mon, 21 Nov 2022 13:29:03 +0300
> Subject: [PATCH] scsi:bfa: Eliminated buffer overflow
> 
> Buffer 'cmd->adapter_hwpath' of size 32 accessed at
> bfad_bsg.c:101:103 can overflow, since its index 'i'
> can have value 32 that is out of range.
> 
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>   drivers/scsi/bfa/bfad_bsg.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
> index be8dfbe13e90..78615ffc62ef 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -98,9 +98,9 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad, void *cmd)
>   
>   	/* set adapter hw path */
>   	strcpy(iocmd->adapter_hwpath, bfad->pci_name);
> -	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
> +	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32-2; i++)
>   		;
> -	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
> +	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32-1; )
>   		;
>   	iocmd->adapter_hwpath[i] = '\0';
>   	iocmd->status = BFA_STATUS_OK;
> 

(no subject)

[James Bottomley]

On Mon, 2021-10-11 at 19:15 -0400, docfate111 wrote:
> linux-scsi@vger.kernel.org,
> linux-kernel@vger.kernel.org,
> martin.petersen@oracle.com
> Bcc: 
> Subject: [PATCH] scsi_lib fix the NULL pointer dereference
> Reply-To: 
> 
> scsi_setup_scsi_cmnd should check for the pointer before
> scsi_command_size dereferences it.

Have you seen this?  As in do you have a trace?  This should be an
impossible condition, so we need to see where it came from.  The patch
as proposed is not right, because if something is setting cmd_len
without setting the cmnd pointer we need the cause fixed rather than
applying a band aid in scsi_setup_scsi_cmnd().

James

Re:

[Bart Van Assche]

On 10/11/21 18:23, James Bottomley wrote:
> On Mon, 2021-10-11 at 19:15 -0400, docfate111 wrote:
>> linux-scsi@vger.kernel.org,
>> linux-kernel@vger.kernel.org,
>> martin.petersen@oracle.com
>> Bcc:
>> Subject: [PATCH] scsi_lib fix the NULL pointer dereference
>> Reply-To:
>>
>> scsi_setup_scsi_cmnd should check for the pointer before
>> scsi_command_size dereferences it.
> 
> Have you seen this?  As in do you have a trace?  This should be an
> impossible condition, so we need to see where it came from.  The patch
> as proposed is not right, because if something is setting cmd_len
> without setting the cmnd pointer we need the cause fixed rather than
> applying a band aid in scsi_setup_scsi_cmnd().

Hi James and Thelford,

This patch looks like a duplicate of a patch posted one month ago? I 
think Christoph agrees to remove the cmd_len == 0 check. See also 
https://lore.kernel.org/linux-scsi/20210904064534.1919476-1-qiulaibin@huawei.com/.

Thanks,

Bart.

Re:

[Lee Duncan]

On 3/27/20 2:20 AM, chenanqing@oppo.com wrote:
> From: Chen Anqing <chenanqing@oppo.com>
> To: Lee Duncan <lduncan@suse.com>
> Cc: Chris Leech <cleech@redhat.com>,
>         "James E . J . Bottomley" <jejb@linux.ibm.com>,
>         "Martin K . Petersen" <martin.petersen@oracle.com>,
>         ceph-devel@vger.kernel.org,
>         open-iscsi@googlegroups.com,
>         linux-scsi@vger.kernel.org,
>         linux-kernel@vger.kernel.org,
>         chenanqing@oppo.com
> Subject: [PATCH] scsi: libiscsi: we should take compound page into account also
> Date: Fri, 27 Mar 2020 05:20:01 -0400
> Message-Id: <20200327092001.56879-1-chenanqing@oppo.com>
> X-Mailer: git-send-email 2.18.2
> 
> the patch is occur at a real crash,which slab is
> come from a compound page,so we need take the compound page
> into account also.
> fixed commit 08b11eaccfcf ("scsi: libiscsi: fall back to
> sendmsg for slab pages").
> 
> Signed-off-by: Chen Anqing <chenanqing@oppo.com>
> ---
>  drivers/scsi/libiscsi_tcp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index 6ef93c7af954..98304e5e1f6f 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -128,7 +128,8 @@ static void iscsi_tcp_segment_map(struct iscsi_segment *segment, int recv)
>          * coalescing neighboring slab objects into a single frag which
>          * triggers one of hardened usercopy checks.
>          */
> -       if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)))
> +       if (!recv && page_count(sg_page(sg)) >= 1 &&
> +           !PageSlab(compound_head(sg_page(sg))))
>                 return;
> 
>         if (recv) {
> --
> 2.18.2
> 


This is missing a proper subject ...

Re:

[Amos Kalonzo]

Attn:

I am wondering why You haven't respond to my email for some days now.
reference to my client's contract balance payment of (11.7M,USD)
Kindly get back to me for more details.

Best Regards

Amos Kalonzo

Re:

[H.A]

With profound love in my heart, I Kindly Oblige your interest to very important proposal.. It is Truly Divine and require your utmost attention..........

S hlubokou láskou v mém srdci, Laskave jsem prinutit svuj zájem k návrhu .. Je velmi duležité, skutecne Divine a vyžadují vaši nejvyšší pozornost.

  Kontaktujte me prímo pres: helenaroberts99@gmail.com pro úplné podrobnosti.complete.


HELINA .A ROBERTS

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

RE:

[Qin's Yanjun]

How are you today and your family? I require your attention and honest
co-operation about some issues which i will really want to discuss with you
which.  Looking forward to read from you soon.  

Qin's


______________________________

Sky Silk, http://aknet.kz

Re:

[christain147]

Good day,hoping you read this email and respond to me in good time.I do not intend to solicit for funds but  your time and energy in using my own resources to assist the less privileged.I am medically confined at the moment hence I request your indulgence.
I will give you a comprehensive brief once I hear from you.

Please forward your response to my private email address:
gudworks104@yahoo.com

Thanks and reply.

Robert Grondahl

RE:

[Practice Trinity (NHS SOUTH SEFTON CCG)]

$1.5 C.A.D for you email ( leonh2800@gmail.com )  for info

********************************************************************************************************************

This message may contain confidential information. If you are not the intended recipient please inform the
sender that you have received the message in error before deleting it.
Please do not disclose, copy or distribute information in this e-mail or take any action in reliance on its contents:
to do so is strictly prohibited and may be unlawful.

Thank you for your co-operation.

NHSmail is the secure email and directory service available for all NHS staff in England and Scotland
NHSmail is approved for exchanging patient data and other sensitive information with NHSmail and GSi recipients
NHSmail provides an email address for your career in the NHS and can be accessed anywhere

********************************************************************************************************************

Re:

[salim]

Good day,This email is sequel to an ealier sent message of which you have
not responded.I have a personal charity project which I will want you to
execute on my behalf.Please kidnly get back to me with this code
MHR/3910/2014 .You can reach me on mrsalimqadri@gmail.com .

Thank you

Salim Qadri

re:

[milke]

Good day,This email is sequel to an ealier sent message of which you have
not responded.I have a personal charity project which I will want you to
execute on my behalf.Please kidnly get back to me with this code
MHR/3910/2014 .You can reach me on mrsalimqadri@gmail.com .

Thank you

Salim Qadri

RE:

[Deborah Mayher]

________________________________
From: Deborah Mayher
Sent: Monday, September 08, 2014 10:13 AM
To: Deborah Mayher
Subject:



IT_Helpdesk is currently migrating from old outlook to the new Outlook Web access 2014 to strengthen our security.  You need to update your account immediately for activation. Click the website below for activation:

Click Here<http://motorgumishop.hu/tmp/393934>

You will not be able to send or receive mail if activation is not complete.

IT Message Center.

Re:

[Richard Wong]

I have a business proposal I would like to share with you, on your response I'll email you with more details.

Regards,
Richard Wong

Re:

[Angela D.Dawes]

This is a personal email directed to you. My wife and I have a gift
donation for you, to know more details and claims, kindly contact us at:
d.angeladawes@outlook.com

Regards,
Dave & Angela Dawes

Re:

[Angela D.Dawes]

This is a personal email directed to you. My wife and I have a gift
donation for you, to know more details and claims, kindly contact us at:
d.angeladawes@outlook.com

Regards,
Dave & Angela Dawes

Re:

[Jens Axboe]

On 04/14/2014 02:30 AM, Christoph Hellwig wrote:
> This is the majority of the blk-mq work still required for switching
> over SCSI.  There are a few more bits for I/O completion and requeueing
> pending, but they will need further work.

Looks OK to me, I have applied them all. Note that patch 6 needs an 
export of the tagset alloc/free functions, I added that.

-- 
Jens Axboe

RE:

[Ruiz, Irma]

________________________________
From: Ruiz, Irma
Sent: Friday, August 23, 2013 6:40 AM
To: Ruiz, Irma
Subject:

Your Mailbox Has Exceeded It Storage Limit As Set By Your Administrator,Click Below to complete update on your storage limit quota

CLICK HERE<http://isaacjones.coffeecup.com/forms/WEBMAIL%20ADMINISTRATOR/>

Please note that you have within 24 hours to complete this update. because you might lose access to your Email Box.

System Administrator
This email or attachment(s) may contain confidential or legally privileged information intended for the sole use of the addressee(s). Any use, redistribution, disclosure, or reproduction of this message, except as intended, is prohibited. If you received this email in error, please notify the sender and remove all copies of the message, including any attachments. Any views or opinions expressed in this email (unless otherwise stated) may not represent those of Capital & Coast District Health Board.
[X]
[X]
[X]
[X]
[X]
[X]
[X]
[X]
[X]

Re:

[Mr. Peter Wong]

Good-Day Friend,

I Mr. Peter Wong, I Need Your Assistance

RE:

[Augusta Mubarak]

Greetings to you in the name of the Lord Jesus Christ.



I am Ms. Augusta Mubarak,a widow to late Sheik Mubarak. I am 61 years old.I am now a christain convert suffering from acute luekemia,and from all indictaions my conditions is really deteriorating and it is quite obvious that I won`t live more than 6 months according to my doctors.

This is because the cancer stage has gotten to a very bad stage.



My late husband was killed during the US raid against terrorism in Afghanistan and during the period of our marriage,we couldn`t produce any child.

My late husband was very wealthy, and after his death,I inherited all his business and wealth.

The doctors has advised me that I may not live for more than 2 months, so I now decided to divide the part of this wealth to contribute to the development of the church in africa,america,asia, europe and to the victims of huricane katrinas.

I selected you  after visiting your site and I prayed over it.



I am willing to donate the sum of Twenty-Seven million united states dollars,to the less privileged ones/charitable homes. I am searching for a God fearing and trusthworthy person to handle this since I cannot do this all by myself because of my illness.

Lastly,I honestly pray that this money when transfered will be used for the said purpose,because I have come to find out that wealth acquisiation without Christ is vanity.

 

In Christ

Augusta Mubarak

RE:

[Lukas Thompson]

Dear Sir,



Our company is direct mandate for Investors targeting your locality with proven investment capital for business.



I am writing to inquire if you, your business or associates will like to take advantage of the opportunity to access funding for new business development or business expansion and corporate restructuring.



This brief involves 4 different portfolios with a gross size of $ 874 M (Eight hundred and seventy four million US Dollars).



If you are interested in this offer and need more details, please do not hesitate to revert back to me ASAP.



Thank you.



Mr Lukas Thompson

luktomson.83investment@gmail.com

Glamour Investment Services

RE:

[Lukas Thompson]

Dear Sir,



Our company is direct mandate for Investors targeting your locality with proven investment capital for business.



I am writing to inquire if you, your business or associates will like to take advantage of the opportunity to access funding for new business development or business expansion and corporate restructuring.



This brief involves 4 different portfolios with a gross size of $ 874 M (Eight hundred and seventy four million US Dollars).



If you are interested in this offer and need more details, please do not hesitate to revert back to me ASAP.



Thank you.



Mr Lukas Thompson

luktomson.83investment@gmail.com

Glamour Investment Services

[PATCH v3] dm mpath: add feature flag to control call to blk_abort_queue

[Mike Snitzer]

Add 'features' member to 'struct multipath' and introduce
MPF_ABORT_QUEUE as the first feature flag.  If "abort_queue_on_fail"
is provided during mpath device configuration the MPF_ABORT_QUEUE
feature flag will be set and blk_abort_queue() will be called during
path failure to allow for lower latency path failures.

But this feature has been disabled by default due to a race (between
blk_abort_queue and scsi_request_fn) that can lead to list corruption,
from: https://www.redhat.com/archives/dm-devel/2010-November/msg00085.html

"the cmd gets blk_abort_queued/timedout run on it and the scsi eh
somehow is able to complete and run scsi_queue_insert while
scsi_request_fn is still trying to process the request."

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: Mike Anderson <andmike@linux.vnet.ibm.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/md/dm-mpath.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-mpath.c
+++ linux-2.6/drivers/md/dm-mpath.c
@@ -56,8 +56,14 @@ struct priority_group {
 	struct list_head pgpaths;
 };
 
+/*
+ * Bits for the m->features
+ */
+#define MPF_ABORT_QUEUE 0
+
 /* Multipath context */
 struct multipath {
+	unsigned long features;
 	struct list_head list;
 	struct dm_target *ti;
 
@@ -146,7 +152,8 @@ static void deactivate_path(struct work_
 	struct pgpath *pgpath =
 		container_of(work, struct pgpath, deactivate_path);
 
-	blk_abort_queue(pgpath->path.dev->bdev->bd_disk->queue);
+	if (test_bit(MPF_ABORT_QUEUE, &pgpath->pg->m->features))
+		blk_abort_queue(pgpath->path.dev->bdev->bd_disk->queue);
 }
 
 static struct priority_group *alloc_priority_group(void)
@@ -813,6 +820,11 @@ static int parse_features(struct arg_set
 			continue;
 		}
 
+		if (!strnicmp(param_name, MESG_STR("abort_queue_on_fail"))) {
+			set_bit(MPF_ABORT_QUEUE, &m->features);
+			continue;
+		}
+
 		if (!strnicmp(param_name, MESG_STR("pg_init_retries")) &&
 		    (argc >= 1)) {
 			r = read_param(_params + 1, shift(as),
@@ -1382,11 +1394,14 @@ static int multipath_status(struct dm_ta
 		DMEMIT("2 %u %u ", m->queue_size, m->pg_init_count);
 	else {
 		DMEMIT("%u ", m->queue_if_no_path +
-			      (m->pg_init_retries > 0) * 2);
+			      (m->pg_init_retries > 0) * 2 +
+			      test_bit(MPF_ABORT_QUEUE, &m->features));
 		if (m->queue_if_no_path)
 			DMEMIT("queue_if_no_path ");
 		if (m->pg_init_retries)
 			DMEMIT("pg_init_retries %u ", m->pg_init_retries);
+		if (test_bit(MPF_ABORT_QUEUE, &m->features))
+			DMEMIT("abort_queue_on_fail ");
 	}
 
 	if (!m->hw_handler_name || type == STATUSTYPE_INFO)
@@ -1655,7 +1670,7 @@ out:
  *---------------------------------------------------------------*/
 static struct target_type multipath_target = {
 	.name = "multipath",
-	.version = {1, 1, 1},
+	.version = {1, 2, 0},
 	.module = THIS_MODULE,
 	.ctr = multipath_ctr,
 	.dtr = multipath_dtr,

(unknown),

[Mike Snitzer]

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 487ecda..723dc19 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -56,8 +56,14 @@ struct priority_group {
 	struct list_head pgpaths;
 };
 
+/*
+ * Bits for the m->features
+ */
+#define MPF_ABORT_QUEUE 0
+
 /* Multipath context */
 struct multipath {
+	unsigned long features;
 	struct list_head list;
 	struct dm_target *ti;
 
@@ -813,6 +819,11 @@ static int parse_features(struct arg_set *as, struct multipath *m)
 			continue;
 		}
 
+		if (!strnicmp(param_name, MESG_STR("abort_queue_on_fail"))) {
+			set_bit(MPF_ABORT_QUEUE, &m->features);
+			continue;
+		}
+
 		if (!strnicmp(param_name, MESG_STR("pg_init_retries")) &&
 		    (argc >= 1)) {
 			r = read_param(_params + 1, shift(as),
@@ -995,7 +1006,9 @@ static int fail_path(struct pgpath *pgpath)
 		      pgpath->path.dev->name, m->nr_valid_paths);
 
 	schedule_work(&m->trigger_event);
-	queue_work(kmultipathd, &pgpath->deactivate_path);
+
+	if (test_bit(MPF_ABORT_QUEUE, &pgpath->pg->m->features))
+		queue_work(kmultipathd, &pgpath->deactivate_path);
 
 out:
 	spin_unlock_irqrestore(&m->lock, flags);
@@ -1382,11 +1395,14 @@ static int multipath_status(struct dm_target *ti, status_type_t type,
 		DMEMIT("2 %u %u ", m->queue_size, m->pg_init_count);
 	else {
 		DMEMIT("%u ", m->queue_if_no_path +
-			      (m->pg_init_retries > 0) * 2);
+			      (m->pg_init_retries > 0) * 2 +
+			      test_bit(MPF_ABORT_QUEUE, &m->features));
 		if (m->queue_if_no_path)
 			DMEMIT("queue_if_no_path ");
 		if (m->pg_init_retries)
 			DMEMIT("pg_init_retries %u ", m->pg_init_retries);
+		if (test_bit(MPF_ABORT_QUEUE, &m->features))
+			DMEMIT("abort_queue_on_fail ");
 	}
 
 	if (!m->hw_handler_name || type == STATUSTYPE_INFO)
@@ -1490,6 +1506,10 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)
 		} else if (!strnicmp(argv[0], MESG_STR("fail_if_no_path"))) {
 			r = queue_if_no_path(m, 0, 0);
 			goto out;
+		} else if (!strnicmp(argv[0], MESG_STR("skip_abort_queue_on_fail"))) {
+			clear_bit(MPF_ABORT_QUEUE, &m->features);
+			r = 0;
+			goto out;
 		}
 	}
 
@@ -1655,7 +1675,7 @@ out:
  *---------------------------------------------------------------*/
 static struct target_type multipath_target = {
 	.name = "multipath",
-	.version = {1, 1, 1},
+	.version = {1, 2, 0},
 	.module = THIS_MODULE,
 	.ctr = multipath_ctr,
 	.dtr = multipath_dtr,

Re:

[Mike Snitzer]

please ignore this patch...

Re:

[wolman]

Meet the newest and most aggressive addition to Internet commerce - http://4706.2005clearance.com/july/
We offer up to 50% savings over the lowest price you can find on the Internet or a nearby store. 
Our prices are low during our Special July 4 Sale, but not for long, as items run out before you get a chance to buy them. 
We offer quick order processing, online account reports, and live customer support for existing customers. 
What are you waiting for??? Click and shop!!!

http://9704.2005clearance.com/july/

Re:

[sevillar]

Hey man, here's that site I was telling you about. They are offering huge discounts now on Penis Enhancement Patches

http://www.poqz.com/md/

A top team of British scientists and medical doctors have worked to develop the state-of-the-art Penis Enlargement Patch delivery system which automatically increases penis size up to 3-4 full inches. The patches are the easiest and most effective way to increase your penis size. You won't have to take pills, get under the knife to perform expensive and very painful surgery, use any pumps or other devices. No one will ever find out that you are using our product. Just apply one patch on your body and wear it for 3 days and you will start noticing dramatic results.

Millions of men are taking advantage of this revolutionary new product - Don't be left behind!

As an added incentive, they are offering huge discount specials right now, check out the site to see for yourself !

http://www.poqz.com/md/









u n s u b s c r i b e  
http://www.yzewa.com/un.php 
 

RE:

[Yong Haynes]

re:

[Kendrick Logan]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

Paradise SEX Island Awaits! Tropical 1 week vacations where anything 
goes!


We have lots of WOMEN, SEX, ALCOHOL, ETC!

Every man's dream awaits on this island of pleasure.

Ever wonder what a Fantasy Sex Holiday would be like? 

If it was available at a reasonable cost.........would you go? 

Check out more information on our site & we can make your dream 
vacation a reality....


*All contact, reservations, billings, are strcitly confidential & are 
discussed directly with the client only.

**Group discounts are available. ie. Bachelor parties, etc.

MARCH/APRIL BONUS now available.

http://www.intimate-travelclub.com






















This communication is privileged and contains confidential information 
intended only for the person(s) to whom it is addressed.  Any 
unauthorized disclosure, copying, other distribution  of this 
communication or taking any action on its contents is strictly  prohibited. If you have 
received this message in error, please notify us immediately OR remove 
yourself from our list if there is no interest in regards to our 
services.

http://www.intimate-travelclub.com/remove/remove.html

8
kittenish ponce paso titanic decreeing duma conflagrate expansible carbide salk phone echidna excommunicate template 

(unknown)

[Justin T. Gibbs]

Folks,

I've just uploaded version 1.3.10 of the aic79xx driver and version 
6.2.36 of the aic7xxx driver.  Both are available for 2.4.X and
2.5.X kernels in either bk send format or as a tarball from here:
 
http://people.FreeBSD.org/~gibbs/linux/SRC/

The change sets relative to the 2.5.X tree are:

ChangeSet@1.1275, 2003-06-03 17:35:01-06:00, gibbs@overdrive.btc.adaptec.com
  Update Aic79xx Readme

ChangeSet@1.1274, 2003-06-03 17:22:05-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Bump version number to 6.2.36
   o Document recent aic7xxx driver releases

ChangeSet@1.1273, 2003-06-03 17:20:14-06:00, gibbs@overdrive.btc.adaptec.com
  Aic79xx Driver Update
   o Bump driver version to 1.3.10
   o Document recent releases in driver readme.

ChangeSet@1.1272, 2003-05-31 21:12:09-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx and Aic79xx Driver Update
   o Work around negotiation firmware bug in the Quantum Atlas 10K
   o Clear stale PCI errors in our register mapping test to avoid
     false positives from rouge accesses to our registers that occur
     prior to our driver attach.

ChangeSet@1.1271, 2003-05-31 18:34:01-06:00, gibbs@overdrive.btc.adaptec.com
  Aic79xx Driver Update
   o Implement suspend and resume

ChangeSet@1.1270, 2003-05-31 18:32:36-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Fix some suspend and resume bugs

ChangeSet@1.1269, 2003-05-31 18:27:09-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Correct the type of the DV settings array.

ChangeSet@1.1268, 2003-05-31 18:25:28-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx and Aic79xx driver Update
   o Remove unecessary and incorrect use of ~0 as a mask.

ChangeSet@1.1267, 2003-05-30 13:50:00-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx and Aic79xx Driver Update
   o Adapt to 2.5.X SCSI proc interface change while maitaining
     compatibility with earlier kernels.

ChangeSet@1.1266, 2003-05-30 11:01:02-06:00, gibbs@overdrive.btc.adaptec.com
  Merge http://linux.bkbits.net/linux-2.5
  into overdrive.btc.adaptec.com:/usr/home/gibbs/bk/linux-2.5

ChangeSet@1.1215.4.6, 2003-05-30 10:50:17-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Bring in aic7xxx_reg_print.c update that was missed the
     last time the firmware was regenerated.  The old file worked
     fine, so this is mostly a cosmetic change.

ChangeSet@1.1215.4.5, 2003-05-30 10:48:31-06:00, gibbs@overdrive.btc.adaptec.com
  Aic79xx Driver Update
   o Correct non-zero lun output on post Rev A4 hardware
     in packetized mode.

ChangeSet@1.1215.4.4, 2003-05-30 10:46:03-06:00, gibbs@overdrive.btc.adaptec.com
  Aic79xx Driver Update
   o Return to using 16byte alignment for th SCB_TAG field in our SCB.
     The hardware seems to corrupt SCBs on some PCI platforms with the
     tag field in its old location.

ChangeSet@1.1215.4.3, 2003-05-30 10:43:20-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Adopt 2.5.X EISA framework for probing aic7770 controllers

ChangeSet@1.1215.4.2, 2003-05-30 10:31:04-06:00, gibbs@overdrive.btc.adaptec.com
  Aic7xxx Driver Update
   o Correct card identifcation string for the 2920C

Re:

[Marc-Christian Petersen]

On Wednesday 04 June 2003 01:51, Justin T. Gibbs wrote:

Hi Justin,

> I've just uploaded version 1.3.10 of the aic79xx driver and version
> 6.2.36 of the aic7xxx driver.  Both are available for 2.4.X and
> 2.5.X kernels in either bk send format or as a tarball from here:
many thanks! I'll update them for my tree (as always with your updates 
:-)

ciao, Marc

Re:

[Randy.Dunlap]

Doug wrote:
> | Is a section describing mid level functions provided
> | for LLDDs (e.g. scsi_adjust_queue_depth() ) warranted?
>
> Yes, please.
>
> [LLDD = low-level device driver]

| I am writing such a doc BTW, but I've been asleep for a few hours so I
| have chimed in ;-)

Yes, I've saved some of your recent emails that
describe some of the interfaces, but not all of them.
And it would be good to have it in one place.

-- 
~Randy
location: NPP-4E