Re: [Ksummit-2012-discuss] [ATTEND] Your upstream maintainer just isn't that into you...

Re: [Ksummit-2012-discuss] [ATTEND] Your upstream maintainer just isn't that into you...

[Dan Carpenter]

On Mon, Jun 25, 2012 at 06:44:56PM -0700, Dan Williams wrote:
> Can we do a better job of bounding the maximum latency for acceptance
> or rejection of a patch?

I sent quite a few patches all over the kernel as part of my Smatch
work.  If I don't get feedback on my patch then I assume it will be
merged.  I've gone through my sent-mail box and collected the
patches which weren't merged.  Hopefully, this is useful in
discussing maintainer submitter feedback.

Some of these patches are probably wrong but it would have been nice
to get some feedback on that.

The SCSI subsystem obviously stands out as pretty bad.  It's the
only subsystem where you can send a patch over and over and not get
a response.  It's not that I think all my patches should be merged
without any review and free hugs for everyone...  For all the other
patches in this list, I feel that the patches was dropped by
mistake but in SCSI it was silently ignored as a policy.

Causes of patches being lost:
* Poor changelog on a cleanup patch.
* I didn't CC the person who introduced the bug so I never got
  their acked-by.
* The patch wasn't obvious and the maintainer was planning to review
  it later and forgot.
* Confusion about who was supposed to pull a patch.
* Poor communication between the maintainer and me and I didn't
  realize they wanted me to redo the patch.  Don't phrase your
  statements as questions.

regards,
dan carpenter

[patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

[Dan Carpenter]

If mc == BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Wed, 6 Jul 2011.

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
 			return;
 		}
 
-		if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
+		if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
 			return;
 
 		mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);

[patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

[Dan Carpenter]

If mc = BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Wed, 6 Jul 2011.

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
 			return;
 		}
 
-		if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn = NULL))
+		if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn = NULL))
 			return;
 
 		mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);

[patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.

I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto.  I looked at other functions in
this file and it seems like returning directly is standard.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is the third time I have sent this patch.  It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.

diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
 bfad_im_probe(struct bfad_s *bfad)
 {
 	struct bfad_im_s      *im;
-	bfa_status_t    rc = BFA_STATUS_OK;
 
 	im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
-	if (im == NULL) {
-		rc = BFA_STATUS_ENOMEM;
-		goto ext;
-	}
+	if (im == NULL)
+		return BFA_STATUS_ENOMEM;
 
 	bfad->im = im;
 	im->bfad = bfad;
 
 	if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
 		kfree(im);
-		rc = BFA_STATUS_FAILED;
+		return BFA_STATUS_FAILED;
 	}
 
 	INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
-	return rc;
+	return BFA_STATUS_OK;
 }
 
 void

[patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

[Dan Carpenter]

If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.

I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto.  I looked at other functions in
this file and it seems like returning directly is standard.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is the third time I have sent this patch.  It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.

diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
 bfad_im_probe(struct bfad_s *bfad)
 {
 	struct bfad_im_s      *im;
-	bfa_status_t    rc = BFA_STATUS_OK;
 
 	im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
-	if (im = NULL) {
-		rc = BFA_STATUS_ENOMEM;
-		goto ext;
-	}
+	if (im = NULL)
+		return BFA_STATUS_ENOMEM;
 
 	bfad->im = im;
 	im->bfad = bfad;
 
 	if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
 		kfree(im);
-		rc = BFA_STATUS_FAILED;
+		return BFA_STATUS_FAILED;
 	}
 
 	INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
-	return rc;
+	return BFA_STATUS_OK;
 }
 
 void

[patch -resend] [SCSI] megaraid: remove a spurious IRQ enable

[Dan Carpenter]

We took this lock with spin_lock() so we should unlock it with
spin_unlock() instead of spin_unlock_irq().  This was introduced in
f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This was originally sent on Sat, 30 Jul 2011.  I have cleaned up the
commit message a bit and added Christoph to the CC list.

diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
index 35bd138..54b1c5b 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
 	}
 
  out:
-	spin_unlock_irq(&adapter->lock);
+	spin_unlock(&adapter->lock);
 	return rval;
 }
 

[patch -resend] [SCSI] megaraid: remove a spurious IRQ enable

[Dan Carpenter]

We took this lock with spin_lock() so we should unlock it with
spin_unlock() instead of spin_unlock_irq().  This was introduced in
f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This was originally sent on Sat, 30 Jul 2011.  I have cleaned up the
commit message a bit and added Christoph to the CC list.

diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
index 35bd138..54b1c5b 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
 	}
 
  out:
-	spin_unlock_irq(&adapter->lock);
+	spin_unlock(&adapter->lock);
 	return rval;
 }
 

[patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[Dan Carpenter]

scsi_dma_map() returns -ENOMEM on error.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Tue, 20 Sep 2011.

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 374c4ed..b2c3a1a 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
 
 	/* Build ASC_SCSI_Q */
 	use_sg = scsi_dma_map(scp);
+	if (use_sg < 0) {
+		scsi_dma_unmap(scp);
+		scp->result = HOST_BYTE(DID_SOFT_ERROR);
+		return ASC_ERROR;
+	}
+
 	if (use_sg != 0) {
 		int sgcnt;
 		struct scatterlist *slp;

[patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[Dan Carpenter]

scsi_dma_map() returns -ENOMEM on error.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Tue, 20 Sep 2011.

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 374c4ed..b2c3a1a 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
 
 	/* Build ASC_SCSI_Q */
 	use_sg = scsi_dma_map(scp);
+	if (use_sg < 0) {
+		scsi_dma_unmap(scp);
+		scp->result = HOST_BYTE(DID_SOFT_ERROR);
+		return ASC_ERROR;
+	}
+
 	if (use_sg != 0) {
 		int sgcnt;
 		struct scatterlist *slp;

[patch 2/2 -resend] SCSI: advansys: use a subsystem error code

[Dan Carpenter]

This was supposed to be a subsystem specific error code.  It makes
static checkers complain because "warn_code" is unsigned short so it
can't store negatives.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Tue, 20 Sep 2011.

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index b2c3a1a..4212586 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -4724,7 +4724,7 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(board->dev, asc_dvc->overrun_dma)) {
-		warn_code = -ENOMEM;
+		warn_code = UW_ERR;
 		goto err_dma_map;
 	}
 	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);

[patch 2/2 -resend] SCSI: advansys: use a subsystem error code

[Dan Carpenter]

This was supposed to be a subsystem specific error code.  It makes
static checkers complain because "warn_code" is unsigned short so it
can't store negatives.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Tue, 20 Sep 2011.

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index b2c3a1a..4212586 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -4724,7 +4724,7 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(board->dev, asc_dvc->overrun_dma)) {
-		warn_code = -ENOMEM;
+		warn_code = UW_ERR;
 		goto err_dma_map;
 	}
 	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);

[patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[Dan Carpenter]

I don't think we're actually likely to hit this limit but if we do
then the comparison should be done as size_t.  The original code
is equivalent to:
        len = strlen(sptr) % USHRT_MAX;

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I was told this patch "has already made it upstream via the v9fs pull."
but it must have been dropped accidentally.  Originally sent on Sat,
Jan 15, 2011.

diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 9ee48cb..3d33ecf 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
 				const char *sptr = va_arg(ap, const char *);
 				uint16_t len = 0;
 				if (sptr)
-					len = min_t(uint16_t, strlen(sptr),
+					len = min_t(size_t, strlen(sptr),
 								USHRT_MAX);
 
 				errcode = p9pdu_writef(pdu, proto_version,

[patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[Dan Carpenter]

I don't think we're actually likely to hit this limit but if we do
then the comparison should be done as size_t.  The original code
is equivalent to:
        len = strlen(sptr) % USHRT_MAX;

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I was told this patch "has already made it upstream via the v9fs pull."
but it must have been dropped accidentally.  Originally sent on Sat,
Jan 15, 2011.

diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 9ee48cb..3d33ecf 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
 				const char *sptr = va_arg(ap, const char *);
 				uint16_t len = 0;
 				if (sptr)
-					len = min_t(uint16_t, strlen(sptr),
+					len = min_t(size_t, strlen(sptr),
 								USHRT_MAX);
 
 				errcode = p9pdu_writef(pdu, proto_version,

[patch -resend] spi/spidev: handle integer wrap in spidev_message()

[Dan Carpenter]

"k_tmp->len" and "total" are unsigned integers.  The first message
could be close to "bufsiz" (4096) and then the next message could be
4GB which would cause an integer overflow.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have a way to test this.  I originally sent this message on Tue,
18 Oct 2011.  I'm not totally sure what the implications are but it
seemed like there might be security implications.  I honestly don't
know.  I never received any feedback on the patch.

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 830adbe..aab05e1 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -241,7 +241,7 @@ static int spidev_message(struct spidev_data *spidev,
 		k_tmp->len = u_tmp->len;
 
 		total += k_tmp->len;
-		if (total > bufsiz) {
+		if (total > bufsiz || total < k_tmp->len) {
 			status = -EMSGSIZE;
 			goto done;
 		}

[patch -resend] spi/spidev: handle integer wrap in spidev_message()

[Dan Carpenter]

"k_tmp->len" and "total" are unsigned integers.  The first message
could be close to "bufsiz" (4096) and then the next message could be
4GB which would cause an integer overflow.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have a way to test this.  I originally sent this message on Tue,
18 Oct 2011.  I'm not totally sure what the implications are but it
seemed like there might be security implications.  I honestly don't
know.  I never received any feedback on the patch.

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 830adbe..aab05e1 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -241,7 +241,7 @@ static int spidev_message(struct spidev_data *spidev,
 		k_tmp->len = u_tmp->len;
 
 		total += k_tmp->len;
-		if (total > bufsiz) {
+		if (total > bufsiz || total < k_tmp->len) {
 			status = -EMSGSIZE;
 			goto done;
 		}

[patch -resend] mmc: ushc: fix an endianness conversion in ushc_request()

[Dan Carpenter]

The ->cmd_idx field is 8 bits, not 16 so the call to cpu_to_le16()
will probably set cmd_idx to zero here on big endian systems.  My guess
is that this works because it has been tested on little endian systems
where the conversion to le16 is a nop.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have a way to test this.

I have added a sentence to the changelog.  I have added David Vrabel to
the CC list.  This was originally sent to the mailing list on Mon, 21
Nov 2011 and there was no response.

diff --git a/drivers/mmc/host/ushc.c b/drivers/mmc/host/ushc.c
index c0105a2..25932f5 100644
--- a/drivers/mmc/host/ushc.c
+++ b/drivers/mmc/host/ushc.c
@@ -278,7 +278,7 @@ static void ushc_request(struct mmc_host *mmc, struct mmc_request *req)
 	ushc->current_req = req;
 
 	/* Start cmd with CBW. */
-	ushc->cbw->cmd_idx = cpu_to_le16(req->cmd->opcode);
+	ushc->cbw->cmd_idx = req->cmd->opcode;
 	if (req->data)
 		ushc->cbw->block_size = cpu_to_le16(req->data->blksz);
 	else

[patch -resend] mmc: ushc: fix an endianness conversion in ushc_request()

[Dan Carpenter]

The ->cmd_idx field is 8 bits, not 16 so the call to cpu_to_le16()
will probably set cmd_idx to zero here on big endian systems.  My guess
is that this works because it has been tested on little endian systems
where the conversion to le16 is a nop.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have a way to test this.

I have added a sentence to the changelog.  I have added David Vrabel to
the CC list.  This was originally sent to the mailing list on Mon, 21
Nov 2011 and there was no response.

diff --git a/drivers/mmc/host/ushc.c b/drivers/mmc/host/ushc.c
index c0105a2..25932f5 100644
--- a/drivers/mmc/host/ushc.c
+++ b/drivers/mmc/host/ushc.c
@@ -278,7 +278,7 @@ static void ushc_request(struct mmc_host *mmc, struct mmc_request *req)
 	ushc->current_req = req;
 
 	/* Start cmd with CBW. */
-	ushc->cbw->cmd_idx = cpu_to_le16(req->cmd->opcode);
+	ushc->cbw->cmd_idx = req->cmd->opcode;
 	if (req->data)
 		ushc->cbw->block_size = cpu_to_le16(req->data->blksz);
 	else

[patch -resend] sgi-xp: nested calls to spin_lock_irqsave()

[Dan Carpenter]

The code here has a nested spin_lock_irqsave().  It's not needed since
IRQs are already disabled and it causes a problem because it means that
IRQs won't be enabled again at the end.  The second call to
spin_lock_irqsave() will overwrite the value of irq_flags and we can't
restore the proper settings.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have added Jack Steiner to the CC list.  This was originally sent on
Thu, 15 Dec 2011.

diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
index 17bbacb..87b251a 100644
--- a/drivers/misc/sgi-xp/xpc_uv.c
+++ b/drivers/misc/sgi-xp/xpc_uv.c
@@ -452,9 +452,9 @@ xpc_handle_activate_mq_msg_uv(struct xpc_partition *part,
 
 		if (msg->activate_gru_mq_desc_gpa !=
 		    part_uv->activate_gru_mq_desc_gpa) {
-			spin_lock_irqsave(&part_uv->flags_lock, irq_flags);
+			spin_lock(&part_uv->flags_lock);
 			part_uv->flags &= ~XPC_P_CACHED_ACTIVATE_GRU_MQ_DESC_UV;
-			spin_unlock_irqrestore(&part_uv->flags_lock, irq_flags);
+			spin_unlock(&part_uv->flags_lock);
 			part_uv->activate_gru_mq_desc_gpa =
 			    msg->activate_gru_mq_desc_gpa;
 		}

[patch -resend] sgi-xp: nested calls to spin_lock_irqsave()

[Dan Carpenter]

The code here has a nested spin_lock_irqsave().  It's not needed since
IRQs are already disabled and it causes a problem because it means that
IRQs won't be enabled again at the end.  The second call to
spin_lock_irqsave() will overwrite the value of irq_flags and we can't
restore the proper settings.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have added Jack Steiner to the CC list.  This was originally sent on
Thu, 15 Dec 2011.

diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
index 17bbacb..87b251a 100644
--- a/drivers/misc/sgi-xp/xpc_uv.c
+++ b/drivers/misc/sgi-xp/xpc_uv.c
@@ -452,9 +452,9 @@ xpc_handle_activate_mq_msg_uv(struct xpc_partition *part,
 
 		if (msg->activate_gru_mq_desc_gpa ! 		    part_uv->activate_gru_mq_desc_gpa) {
-			spin_lock_irqsave(&part_uv->flags_lock, irq_flags);
+			spin_lock(&part_uv->flags_lock);
 			part_uv->flags &= ~XPC_P_CACHED_ACTIVATE_GRU_MQ_DESC_UV;
-			spin_unlock_irqrestore(&part_uv->flags_lock, irq_flags);
+			spin_unlock(&part_uv->flags_lock);
 			part_uv->activate_gru_mq_desc_gpa  			    msg->activate_gru_mq_desc_gpa;
 		}

[patch 1/3 -resend] [SCSI] pmcraid: remove unneeded check

[Dan Carpenter]

request_size is zero here so this condition is always false.  Also we
already handled negative values some lines earlier so it's not negative
for that reason as well.

It looks like this was introduced because we applied Dan Rosenberg's fix
twice by mistake:
b5b515445f4 "[SCSI] pmcraid: reject negative request size"
5f6279da376 "[SCSI] pmcraid: reject negative request size"

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have tweaked the changelog slightly.  This was originally sent on
Fri, 16 Dec 2011.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index ea8a0b4..d81a159 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3870,9 +3870,6 @@ static long pmcraid_ioctl_passthrough(
 			pmcraid_err("couldn't build passthrough ioadls\n");
 			goto out_free_buffer;
 		}
-	} else if (request_size < 0) {
-		rc = -EINVAL;
-		goto out_free_buffer;
 	}
 
 	/* If data is being written into the device, copy the data from user

[patch 1/3 -resend] [SCSI] pmcraid: remove unneeded check

[Dan Carpenter]

request_size is zero here so this condition is always false.  Also we
already handled negative values some lines earlier so it's not negative
for that reason as well.

It looks like this was introduced because we applied Dan Rosenberg's fix
twice by mistake:
b5b515445f4 "[SCSI] pmcraid: reject negative request size"
5f6279da376 "[SCSI] pmcraid: reject negative request size"

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have tweaked the changelog slightly.  This was originally sent on
Fri, 16 Dec 2011.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index ea8a0b4..d81a159 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3870,9 +3870,6 @@ static long pmcraid_ioctl_passthrough(
 			pmcraid_err("couldn't build passthrough ioadls\n");
 			goto out_free_buffer;
 		}
-	} else if (request_size < 0) {
-		rc = -EINVAL;
-		goto out_free_buffer;
 	}
 
 	/* If data is being written into the device, copy the data from user

[patch 2/3 -resend] [SCSI] pmcraid: cpu_to_le32() => cpu_to_le64()

[Dan Carpenter]

The cpu_to_le32() truncates the address to 32 bits.  All the other
places that set .address use cpu_to_le64().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
What about if dma_addr_t is 32 bits on a big endian system?  Can that
happen?

This patch was originally sent on Fri, 16 Dec 2011 and I sent a second
reminder on Mon, Apr 30, 2012.  I was silently ignored both times.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index d81a159..a38dade 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -1242,7 +1242,7 @@ static struct pmcraid_cmd *pmcraid_init_hcam
 
 	ioadl[0].flags |= IOADL_FLAGS_READ_LAST;
 	ioadl[0].data_len = cpu_to_le32(rcb_size);
-	ioadl[0].address = cpu_to_le32(dma);
+	ioadl[0].address = cpu_to_le64(dma);
 
 	cmd->cmd_done = cmd_done;
 	return cmd;

[patch 2/3 -resend] [SCSI] pmcraid: cpu_to_le32() => cpu_to_le64()

[Dan Carpenter]

The cpu_to_le32() truncates the address to 32 bits.  All the other
places that set .address use cpu_to_le64().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
What about if dma_addr_t is 32 bits on a big endian system?  Can that
happen?

This patch was originally sent on Fri, 16 Dec 2011 and I sent a second
reminder on Mon, Apr 30, 2012.  I was silently ignored both times.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index d81a159..a38dade 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -1242,7 +1242,7 @@ static struct pmcraid_cmd *pmcraid_init_hcam
 
 	ioadl[0].flags |= IOADL_FLAGS_READ_LAST;
 	ioadl[0].data_len = cpu_to_le32(rcb_size);
-	ioadl[0].address = cpu_to_le32(dma);
+	ioadl[0].address = cpu_to_le64(dma);
 
 	cmd->cmd_done = cmd_done;
 	return cmd;

[patch 3/3 -resend] [SCSI] pmcraid: find_first_zero_bit() takes bits not bytes

[Dan Carpenter]

The find_first_zero_bit() function takes the size in bits not bytes.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This patch was originally sent on Wed, 2 May 2012.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index a38dade..719619a 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -5376,7 +5376,7 @@ static unsigned short pmcraid_get_minor(void)
 {
 	int minor;
 
-	minor = find_first_zero_bit(pmcraid_minor, sizeof(pmcraid_minor));
+	minor = find_first_zero_bit(pmcraid_minor, PMCRAID_MAX_ADAPTERS);
 	__set_bit(minor, pmcraid_minor);
 	return minor;
 }

[patch 3/3 -resend] [SCSI] pmcraid: find_first_zero_bit() takes bits not bytes

[Dan Carpenter]

The find_first_zero_bit() function takes the size in bits not bytes.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This patch was originally sent on Wed, 2 May 2012.

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index a38dade..719619a 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -5376,7 +5376,7 @@ static unsigned short pmcraid_get_minor(void)
 {
 	int minor;
 
-	minor = find_first_zero_bit(pmcraid_minor, sizeof(pmcraid_minor));
+	minor = find_first_zero_bit(pmcraid_minor, PMCRAID_MAX_ADAPTERS);
 	__set_bit(minor, pmcraid_minor);
 	return minor;
 }

[patch -resend] [SCSI] isci: add a couple __iomem annotations

[Dan Carpenter]

These are __iomem.  Sparse complains if we don't have that.

drivers/scsi/isci/phy.c +149 70: warning:
        incorrect type in initializer (different address spaces)

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Sent on Wed, 18 Jan 2012.

diff --git a/drivers/scsi/isci/host.c b/drivers/scsi/isci/host.c
index bc8981e..4a095f3 100644
--- a/drivers/scsi/isci/host.c
+++ b/drivers/scsi/isci/host.c
@@ -1973,7 +1973,7 @@ static void sci_controller_afe_initialization(struct isci_host *ihost)
 	}
 
 	for (phy_id = 0; phy_id < SCI_MAX_PHYS; phy_id++) {
-		struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_id];
+		struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_id];
 		const struct sci_phy_oem_params *oem_phy = &oem->phys[phy_id];
 		int cable_length_long =
 			is_long_cable(phy_id, cable_selection_mask);
diff --git a/drivers/scsi/isci/phy.c b/drivers/scsi/isci/phy.c
index 18f43d4..7b60353 100644
--- a/drivers/scsi/isci/phy.c
+++ b/drivers/scsi/isci/phy.c
@@ -169,7 +169,7 @@ sci_phy_link_layer_initialization(struct isci_phy *iphy,
 	phy_cap.gen1_no_ssc = 1;
 	if (ihost->oem_parameters.controller.do_enable_ssc) {
 		struct scu_afe_registers __iomem *afe = &ihost->scu_registers->afe;
-		struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_idx];
+		struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_idx];
 		struct isci_pci_info *pci_info = to_pci_info(ihost->pdev);
 		bool en_sas = false;
 		bool en_sata = false;

[patch -resend] [SCSI] isci: add a couple __iomem annotations

[Dan Carpenter]

These are __iomem.  Sparse complains if we don't have that.

drivers/scsi/isci/phy.c +149 70: warning:
        incorrect type in initializer (different address spaces)

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Sent on Wed, 18 Jan 2012.

diff --git a/drivers/scsi/isci/host.c b/drivers/scsi/isci/host.c
index bc8981e..4a095f3 100644
--- a/drivers/scsi/isci/host.c
+++ b/drivers/scsi/isci/host.c
@@ -1973,7 +1973,7 @@ static void sci_controller_afe_initialization(struct isci_host *ihost)
 	}
 
 	for (phy_id = 0; phy_id < SCI_MAX_PHYS; phy_id++) {
-		struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_id];
+		struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_id];
 		const struct sci_phy_oem_params *oem_phy = &oem->phys[phy_id];
 		int cable_length_long  			is_long_cable(phy_id, cable_selection_mask);
diff --git a/drivers/scsi/isci/phy.c b/drivers/scsi/isci/phy.c
index 18f43d4..7b60353 100644
--- a/drivers/scsi/isci/phy.c
+++ b/drivers/scsi/isci/phy.c
@@ -169,7 +169,7 @@ sci_phy_link_layer_initialization(struct isci_phy *iphy,
 	phy_cap.gen1_no_ssc = 1;
 	if (ihost->oem_parameters.controller.do_enable_ssc) {
 		struct scu_afe_registers __iomem *afe = &ihost->scu_registers->afe;
-		struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_idx];
+		struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_idx];
 		struct isci_pci_info *pci_info = to_pci_info(ihost->pdev);
 		bool en_sas = false;
 		bool en_sata = false;

Re: [SCSI] bfa: Implement LUN Masking feature using the SCSI Slave Callouts.

[Dan Carpenter]

Hi,

This bug is still present in linux-next.

regards,
dan carpenter

On Wed, Jan 11, 2012 at 12:32:34PM +0300, Dan Carpenter wrote:
> Hello Krishna Gudipati,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 5b7db7af522d: "[SCSI] bfa: Implement LUN Masking feature 
> using the SCSI Slave Callouts." from Dec 20, 2011, leads to the 
> following Smatch complaint:
> 
> drivers/scsi/bfa/bfad_im.c +962 bfad_im_slave_alloc()
> 	 warn: variable dereferenced before check 'rport' (see line 959)
> 
> drivers/scsi/bfa/bfad_im.c
>    957          struct fc_rport *rport = starget_to_rport(scsi_target(sdev));
>    958		struct bfad_itnim_data_s *itnim_data =
>    959					(struct bfad_itnim_data_s *) rport->dd_data;
>                                                                      ^^^^^^^
> New dereference.
> 
>    960		struct bfa_s *bfa = itnim_data->itnim->bfa_itnim->bfa;
>    961	
>    962		if (!rport || fc_remote_port_chkready(rport))
>                      ^^^^^
> Old check.
> 
>    963			return -ENXIO;
>    964	
> 
> regards,
> dan carpenter
> 

[patch -resend] NVMe: handle allocation failure in nvme_map_user_pages()

[Dan Carpenter]

We should return here and avoid a NULL dereference.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 20 Jan 2012.

diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
index 38a2d06..b4d85b9 100644
--- a/drivers/block/nvme.c
+++ b/drivers/block/nvme.c
@@ -1037,6 +1037,8 @@ static struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 	offset = offset_in_page(addr);
 	count = DIV_ROUND_UP(offset + length, PAGE_SIZE);
 	pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return ERR_PTR(-ENOMEM);
 
 	err = get_user_pages_fast(addr, count, 1, pages);
 	if (err < count) {

[patch -resend] NVMe: handle allocation failure in nvme_map_user_pages()

[Dan Carpenter]

We should return here and avoid a NULL dereference.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 20 Jan 2012.

diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
index 38a2d06..b4d85b9 100644
--- a/drivers/block/nvme.c
+++ b/drivers/block/nvme.c
@@ -1037,6 +1037,8 @@ static struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 	offset = offset_in_page(addr);
 	count = DIV_ROUND_UP(offset + length, PAGE_SIZE);
 	pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return ERR_PTR(-ENOMEM);
 
 	err = get_user_pages_fast(addr, count, 1, pages);
 	if (err < count) {

[patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Dan Carpenter]

The intent here was to test that the flag was clear but the '!' has
higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
equivalent to "&& (!sgs[i].flags) ..."

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
fix on Thu, May 3, 2012.

diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
index 4008b9c..f6f0cf9 100644
--- a/drivers/media/dvb/dvb-usb/az6007.c
+++ b/drivers/media/dvb/dvb-usb/az6007.c
@@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 		addr = msgs[i].addr << 1;
 		if (((i + 1) < num)
 		    && (msgs[i].len == 1)
-		    && (!msgs[i].flags & I2C_M_RD)
+		    && (!(msgs[i].flags & I2C_M_RD))
 		    && (msgs[i + 1].flags & I2C_M_RD)
 		    && (msgs[i].addr == msgs[i + 1].addr)) {
 			/*

[patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Dan Carpenter]

The intent here was to test that the flag was clear but the '!' has
higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
equivalent to "&& (!sgs[i].flags) ..."

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
fix on Thu, May 3, 2012.

diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
index 4008b9c..f6f0cf9 100644
--- a/drivers/media/dvb/dvb-usb/az6007.c
+++ b/drivers/media/dvb/dvb-usb/az6007.c
@@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 		addr = msgs[i].addr << 1;
 		if (((i + 1) < num)
 		    && (msgs[i].len = 1)
-		    && (!msgs[i].flags & I2C_M_RD)
+		    && (!(msgs[i].flags & I2C_M_RD))
 		    && (msgs[i + 1].flags & I2C_M_RD)
 		    && (msgs[i].addr = msgs[i + 1].addr)) {
 			/*

[patch v3 -resend] edac i5000, i5400: fix pointer math in i5000_get_mc_regs()

[Dan Carpenter]

"pvt->ambase" is a u64 datatype.  The intent here is to fill the first
half in the first call to pci_read_config_dword() and the other half in
the second.  Unfortunately the pointer math is wrong so we set the wrong
data.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Redid it as with a union as Walter Harms suggested.
    Fixed the same bug in i5400_edac.c as well.
v3: Make the struct __packed just in case.

I don't have this hardware, so please review carefully.  But the
original code obviously corrupts memory so my patch could hardly be
worse...

This was originally sent on Mon, Mar 5, 2012.  Btw, there are some
Sparse errors in these files.  The code looks buggy but I don't know
what the intent was.

drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value

diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index a5c33df..39c6375 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -328,7 +328,13 @@ struct i5000_pvt {
 	struct pci_dev *branch_1;	/* 22.0 */
 
 	u16 tolm;		/* top of low memory */
-	u64 ambase;		/* AMB BAR */
+	union {
+		u64 ambase;		/* AMB BAR */
+		struct {
+			u32 ambase_bottom;
+			u32 ambase_top;
+		} u __packed;
+	};
 
 	u16 mir0, mir1, mir2;
 
@@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
 	pvt = mci->pvt_info;
 
 	pci_read_config_dword(pvt->system_address, AMBASE,
-			(u32 *) & pvt->ambase);
+			&pvt->u.ambase_bottom);
 	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
-			((u32 *) & pvt->ambase) + sizeof(u32));
+			&pvt->u.ambase_top);
 
 	maxdimmperch = pvt->maxdimmperch;
 	maxch = pvt->maxch;
diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
index 50069c6..2772469 100644
--- a/drivers/edac/i5400_edac.c
+++ b/drivers/edac/i5400_edac.c
@@ -327,7 +327,13 @@ struct i5400_pvt {
 	struct pci_dev *branch_1;		/* 22.0 */
 
 	u16 tolm;				/* top of low memory */
-	u64 ambase;				/* AMB BAR */
+	union {
+		u64 ambase;				/* AMB BAR */
+		struct {
+			u32 ambase_bottom;
+			u32 ambase_top;
+		} u __packed;
+	};
 
 	u16 mir0, mir1;
 
@@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
 	pvt = mci->pvt_info;
 
 	pci_read_config_dword(pvt->system_address, AMBASE,
-			(u32 *) &pvt->ambase);
+			&pvt->u.ambase_bottom);
 	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
-			((u32 *) &pvt->ambase) + sizeof(u32));
+			&pvt->u.ambase_top);
 
 	maxdimmperch = pvt->maxdimmperch;
 	maxch = pvt->maxch;

[patch v3 -resend] edac i5000, i5400: fix pointer math in i5000_get_mc_regs()

[Dan Carpenter]

"pvt->ambase" is a u64 datatype.  The intent here is to fill the first
half in the first call to pci_read_config_dword() and the other half in
the second.  Unfortunately the pointer math is wrong so we set the wrong
data.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Redid it as with a union as Walter Harms suggested.
    Fixed the same bug in i5400_edac.c as well.
v3: Make the struct __packed just in case.

I don't have this hardware, so please review carefully.  But the
original code obviously corrupts memory so my patch could hardly be
worse...

This was originally sent on Mon, Mar 5, 2012.  Btw, there are some
Sparse errors in these files.  The code looks buggy but I don't know
what the intent was.

drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value

diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index a5c33df..39c6375 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -328,7 +328,13 @@ struct i5000_pvt {
 	struct pci_dev *branch_1;	/* 22.0 */
 
 	u16 tolm;		/* top of low memory */
-	u64 ambase;		/* AMB BAR */
+	union {
+		u64 ambase;		/* AMB BAR */
+		struct {
+			u32 ambase_bottom;
+			u32 ambase_top;
+		} u __packed;
+	};
 
 	u16 mir0, mir1, mir2;
 
@@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
 	pvt = mci->pvt_info;
 
 	pci_read_config_dword(pvt->system_address, AMBASE,
-			(u32 *) & pvt->ambase);
+			&pvt->u.ambase_bottom);
 	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
-			((u32 *) & pvt->ambase) + sizeof(u32));
+			&pvt->u.ambase_top);
 
 	maxdimmperch = pvt->maxdimmperch;
 	maxch = pvt->maxch;
diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
index 50069c6..2772469 100644
--- a/drivers/edac/i5400_edac.c
+++ b/drivers/edac/i5400_edac.c
@@ -327,7 +327,13 @@ struct i5400_pvt {
 	struct pci_dev *branch_1;		/* 22.0 */
 
 	u16 tolm;				/* top of low memory */
-	u64 ambase;				/* AMB BAR */
+	union {
+		u64 ambase;				/* AMB BAR */
+		struct {
+			u32 ambase_bottom;
+			u32 ambase_top;
+		} u __packed;
+	};
 
 	u16 mir0, mir1;
 
@@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
 	pvt = mci->pvt_info;
 
 	pci_read_config_dword(pvt->system_address, AMBASE,
-			(u32 *) &pvt->ambase);
+			&pvt->u.ambase_bottom);
 	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
-			((u32 *) &pvt->ambase) + sizeof(u32));
+			&pvt->u.ambase_top);
 
 	maxdimmperch = pvt->maxdimmperch;
 	maxch = pvt->maxch;

[patch -resend] [SCSI] megaraid: cleanup type issue in mega_build_cmd()

[Dan Carpenter]

On 64 bit systems the current code sets 32 bits of "seg" and leaves the
other 32 uninitialized.  It doesn't matter since the variable is never
used.  But it's still messy and we should fix it.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 2 Mar 2012.

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d39a9f..97825f1 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
 	mega_passthru	*pthru;
 	scb_t	*scb;
 	mbox_t	*mbox;
-	long	seg;
+	u32	seg;
 	char	islogical;
 	int	max_ldrv_num;
 	int	channel = 0;
@@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
 
 			/* Calculate Scatter-Gather info */
 			mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
-					(u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
+					(u32 *)&mbox->m_out.xferaddr, &seg);
 
 			return scb;
 

[patch -resend] [SCSI] megaraid: cleanup type issue in mega_build_cmd()

[Dan Carpenter]

On 64 bit systems the current code sets 32 bits of "seg" and leaves the
other 32 uninitialized.  It doesn't matter since the variable is never
used.  But it's still messy and we should fix it.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 2 Mar 2012.

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d39a9f..97825f1 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
 	mega_passthru	*pthru;
 	scb_t	*scb;
 	mbox_t	*mbox;
-	long	seg;
+	u32	seg;
 	char	islogical;
 	int	max_ldrv_num;
 	int	channel = 0;
@@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
 
 			/* Calculate Scatter-Gather info */
 			mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
-					(u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
+					(u32 *)&mbox->m_out.xferaddr, &seg);
 
 			return scb;
 

[patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Dan Carpenter]

Even though it has "bool" in the name, you have pass a u32 pointer to
debugfs_create_bool().  Otherwise you get memory corruption in
write_file_bool().  Fortunately in this case the corruption happens in
an alignment hole between variables so it doesn't cause any problems.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This was sent on Fri, 2 Mar 2012.

It's probably my fault the bug is still there.  I submitted the patch
but the feed back was that debugfs_create_bool() has a stupid API and
should take a pointer to a bool.  I said that yes, probably that is true
but only two places use the current API incorrectly and I fixed the
other place as well.

I assumed that both my patches would be merged and the person who cared
about API would fix the API but neither of my fixes were applied.

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 518aea7..66ce414 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
 static DEFINE_SPINLOCK(free_entries_lock);
 
 /* Global disable flag - will be set in case of an error */
-static bool global_disable __read_mostly;
+static u32 global_disable __read_mostly;
 
 /* Global error count */
 static u32 error_count;
@@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
 
 	global_disable_dent = debugfs_create_bool("disabled", 0444,
 			dma_debug_dent,
-			(u32 *)&global_disable);
+			&global_disable);
 	if (!global_disable_dent)
 		goto out_err;
 

[patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Dan Carpenter]

Even though it has "bool" in the name, you have pass a u32 pointer to
debugfs_create_bool().  Otherwise you get memory corruption in
write_file_bool().  Fortunately in this case the corruption happens in
an alignment hole between variables so it doesn't cause any problems.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This was sent on Fri, 2 Mar 2012.

It's probably my fault the bug is still there.  I submitted the patch
but the feed back was that debugfs_create_bool() has a stupid API and
should take a pointer to a bool.  I said that yes, probably that is true
but only two places use the current API incorrectly and I fixed the
other place as well.

I assumed that both my patches would be merged and the person who cared
about API would fix the API but neither of my fixes were applied.

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 518aea7..66ce414 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
 static DEFINE_SPINLOCK(free_entries_lock);
 
 /* Global disable flag - will be set in case of an error */
-static bool global_disable __read_mostly;
+static u32 global_disable __read_mostly;
 
 /* Global error count */
 static u32 error_count;
@@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
 
 	global_disable_dent = debugfs_create_bool("disabled", 0444,
 			dma_debug_dent,
-			(u32 *)&global_disable);
+			&global_disable);
 	if (!global_disable_dent)
 		goto out_err;
 

[patch 2/2 -resend] iommu/amd: fix type bug in flush code

[Dan Carpenter]

write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory.  Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 2 Mar 2012.  This is the other patch where I
guess I was supposed to modify debugfs_create_bool() to take bool
pointers.

diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 2435555..c1b1d48 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -652,7 +652,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
  * If true, the addresses will be flushed on unmap time, not when
  * they are reused
  */
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
 
 /* Smallest number of PASIDs supported by any IOMMU in the system */
 extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4127b56..92b9267 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -451,7 +451,7 @@ static void amd_iommu_stats_init(void)
 		return;
 
 	de_fflush  = debugfs_create_bool("fullflush", 0444, stats_dir,
-					 (u32 *)&amd_iommu_unmap_flush);
+					 &amd_iommu_unmap_flush);
 
 	amd_iommu_stats_add(&compl_wait);
 	amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c04ddca..a33612f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf;			/* largest PCI device id we have
 					   to handle */
 LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
-bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */

[patch 2/2 -resend] iommu/amd: fix type bug in flush code

[Dan Carpenter]

write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory.  Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Fri, 2 Mar 2012.  This is the other patch where I
guess I was supposed to modify debugfs_create_bool() to take bool
pointers.

diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 2435555..c1b1d48 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -652,7 +652,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
  * If true, the addresses will be flushed on unmap time, not when
  * they are reused
  */
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
 
 /* Smallest number of PASIDs supported by any IOMMU in the system */
 extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4127b56..92b9267 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -451,7 +451,7 @@ static void amd_iommu_stats_init(void)
 		return;
 
 	de_fflush  = debugfs_create_bool("fullflush", 0444, stats_dir,
-					 (u32 *)&amd_iommu_unmap_flush);
+					 &amd_iommu_unmap_flush);
 
 	amd_iommu_stats_add(&compl_wait);
 	amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c04ddca..a33612f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf;			/* largest PCI device id we have
 					   to handle */
 LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
-bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */

[patch 2/2 -resend] iommu/amd: fix type bug in flush code

[Dan Carpenter]

write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory.  Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.

Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
Originally sent on Fri, 2 Mar 2012.  This is the other patch where I
guess I was supposed to modify debugfs_create_bool() to take bool
pointers.

diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 2435555..c1b1d48 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -652,7 +652,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
  * If true, the addresses will be flushed on unmap time, not when
  * they are reused
  */
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
 
 /* Smallest number of PASIDs supported by any IOMMU in the system */
 extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4127b56..92b9267 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -451,7 +451,7 @@ static void amd_iommu_stats_init(void)
 		return;
 
 	de_fflush  = debugfs_create_bool("fullflush", 0444, stats_dir,
-					 (u32 *)&amd_iommu_unmap_flush);
+					 &amd_iommu_unmap_flush);
 
 	amd_iommu_stats_add(&compl_wait);
 	amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c04ddca..a33612f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf;			/* largest PCI device id we have
 					   to handle */
 LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
-bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */

[patch -resend] isci: make function declaration match implementation

[Dan Carpenter]

Sparse complains that we redeclare this with a different type, because
in the .c file we use an enum and in the .h file we declare the
parameter as a u32.  Probably it's best to use an enum in both places.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I originally sent this on Tue, 27 Mar 2012, and Emil Goode sent the same
patch on Fri, May 4, 2012.  Both were silently ignored.

diff --git a/drivers/scsi/isci/remote_node_context.h b/drivers/scsi/isci/remote_node_context.h
index a703b9c..c7ee81d 100644
--- a/drivers/scsi/isci/remote_node_context.h
+++ b/drivers/scsi/isci/remote_node_context.h
@@ -212,7 +212,7 @@ enum sci_status sci_remote_node_context_destruct(struct sci_remote_node_context
 						      scics_sds_remote_node_context_callback callback,
 						      void *callback_parameter);
 enum sci_status sci_remote_node_context_suspend(struct sci_remote_node_context *sci_rnc,
-						     u32 suspend_type,
+						     enum sci_remote_node_suspension_reasons reason,
 						     u32 suspension_code);
 enum sci_status sci_remote_node_context_resume(struct sci_remote_node_context *sci_rnc,
 						    scics_sds_remote_node_context_callback cb_fn,

[patch -resend] isci: make function declaration match implementation

[Dan Carpenter]

Sparse complains that we redeclare this with a different type, because
in the .c file we use an enum and in the .h file we declare the
parameter as a u32.  Probably it's best to use an enum in both places.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I originally sent this on Tue, 27 Mar 2012, and Emil Goode sent the same
patch on Fri, May 4, 2012.  Both were silently ignored.

diff --git a/drivers/scsi/isci/remote_node_context.h b/drivers/scsi/isci/remote_node_context.h
index a703b9c..c7ee81d 100644
--- a/drivers/scsi/isci/remote_node_context.h
+++ b/drivers/scsi/isci/remote_node_context.h
@@ -212,7 +212,7 @@ enum sci_status sci_remote_node_context_destruct(struct sci_remote_node_context
 						      scics_sds_remote_node_context_callback callback,
 						      void *callback_parameter);
 enum sci_status sci_remote_node_context_suspend(struct sci_remote_node_context *sci_rnc,
-						     u32 suspend_type,
+						     enum sci_remote_node_suspension_reasons reason,
 						     u32 suspension_code);
 enum sci_status sci_remote_node_context_resume(struct sci_remote_node_context *sci_rnc,
 						    scics_sds_remote_node_context_callback cb_fn,

[patch -resend] drm/i915/bios: cleanup return type of intel_parse_bios()

[Dan Carpenter]

These are unintuitive.  These are type bool and return -1 casted to true
on failure.  Let's just make it return an int.  The callers don't care,
but let's change this as a cleanup.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
Originally sent on Wed, Mar 28, 2012.  This one is prossibly my fault.
I assumed these would be in the same tree, but apparently I should have
broken them out.  Daniel acked the patch and told me that if I wanted to
I could break it up and he could merge part (or all of it?).  I have no
problem with redoing patches but I understood that to mean it was going
to go through a someone else's tree with his Acked-by.  If you give me
an option, of course I'm going to pick the laziest one.  That is my firm
promise and commitment.

diff --git a/drivers/gpu/drm/gma500/intel_bios.h b/drivers/gpu/drm/gma500/intel_bios.h
index 0a73866..2e95523 100644
--- a/drivers/gpu/drm/gma500/intel_bios.h
+++ b/drivers/gpu/drm/gma500/intel_bios.h
@@ -431,7 +431,7 @@ struct bdb_driver_features {
 	u8 custom_vbt_version;
 } __attribute__((packed));
 
-extern bool psb_intel_init_bios(struct drm_device *dev);
+extern int psb_intel_init_bios(struct drm_device *dev);
 extern void psb_intel_destroy_bios(struct drm_device *dev);
 
 /*
diff --git a/drivers/gpu/drm/gma500/intel_bios.c b/drivers/gpu/drm/gma500/intel_bios.c
index 973d7f6..8d7caf0 100644
--- a/drivers/gpu/drm/gma500/intel_bios.c
+++ b/drivers/gpu/drm/gma500/intel_bios.c
@@ -427,7 +427,7 @@ parse_device_mapping(struct drm_psb_private *dev_priv,
  *
  * Returns 0 on success, nonzero on failure.
  */
-bool psb_intel_init_bios(struct drm_device *dev)
+int psb_intel_init_bios(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index dbda6e3..31c2107 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -476,7 +476,7 @@ struct bdb_edp {
 } __attribute__ ((packed));
 
 void intel_setup_bios(struct drm_device *dev);
-bool intel_parse_bios(struct drm_device *dev);
+int intel_parse_bios(struct drm_device *dev);
 
 /*
  * Driver<->VBIOS interaction occurs through scratch bits in
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 3534593..8c60741 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -692,7 +692,7 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
  *
  * Returns 0 on success, nonzero on failure.
  */
-bool
+int
 intel_parse_bios(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

[patch -resend] drm/i915/bios: cleanup return type of intel_parse_bios()

[Dan Carpenter]

These are unintuitive.  These are type bool and return -1 casted to true
on failure.  Let's just make it return an int.  The callers don't care,
but let's change this as a cleanup.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
Originally sent on Wed, Mar 28, 2012.  This one is prossibly my fault.
I assumed these would be in the same tree, but apparently I should have
broken them out.  Daniel acked the patch and told me that if I wanted to
I could break it up and he could merge part (or all of it?).  I have no
problem with redoing patches but I understood that to mean it was going
to go through a someone else's tree with his Acked-by.  If you give me
an option, of course I'm going to pick the laziest one.  That is my firm
promise and commitment.

diff --git a/drivers/gpu/drm/gma500/intel_bios.h b/drivers/gpu/drm/gma500/intel_bios.h
index 0a73866..2e95523 100644
--- a/drivers/gpu/drm/gma500/intel_bios.h
+++ b/drivers/gpu/drm/gma500/intel_bios.h
@@ -431,7 +431,7 @@ struct bdb_driver_features {
 	u8 custom_vbt_version;
 } __attribute__((packed));
 
-extern bool psb_intel_init_bios(struct drm_device *dev);
+extern int psb_intel_init_bios(struct drm_device *dev);
 extern void psb_intel_destroy_bios(struct drm_device *dev);
 
 /*
diff --git a/drivers/gpu/drm/gma500/intel_bios.c b/drivers/gpu/drm/gma500/intel_bios.c
index 973d7f6..8d7caf0 100644
--- a/drivers/gpu/drm/gma500/intel_bios.c
+++ b/drivers/gpu/drm/gma500/intel_bios.c
@@ -427,7 +427,7 @@ parse_device_mapping(struct drm_psb_private *dev_priv,
  *
  * Returns 0 on success, nonzero on failure.
  */
-bool psb_intel_init_bios(struct drm_device *dev)
+int psb_intel_init_bios(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index dbda6e3..31c2107 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -476,7 +476,7 @@ struct bdb_edp {
 } __attribute__ ((packed));
 
 void intel_setup_bios(struct drm_device *dev);
-bool intel_parse_bios(struct drm_device *dev);
+int intel_parse_bios(struct drm_device *dev);
 
 /*
  * Driver<->VBIOS interaction occurs through scratch bits in
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 3534593..8c60741 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -692,7 +692,7 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
  *
  * Returns 0 on success, nonzero on failure.
  */
-bool
+int
 intel_parse_bios(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

[patch -resend] leds-lp5523: BUG() in error handling in probe()

[Dan Carpenter]

Inside the error handling in lp5523_init_led(), there is a place that
calls to led_classdev_unregister().  When we unregister the LED drivers,
it tries to set the brightness to OFF.  In this driver setting the
brightness is done through a work queue and the work queue hasn't been
initialized yet.

The result is that we trigger a WARN_ON() in the __queue_work().

The fix is to move the INIT_WORK() in front of the call to
lp5523_init_led().

Matt Renzelmann found this using a bug finding tool.

Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have this hardware, so I can't test it.  I originally sent this
on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
a BUG_ON() so I've updated the commit message.

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 857a3e1..e8a2712 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
 		if (pdata->led_config[i].led_current == 0)
 			continue;
 
+		INIT_WORK(&chip->leds[led].brightness_work,
+			lp5523_led_brightness_work);
+
 		ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
 		if (ret) {
 			dev_err(&client->dev, "error initializing leds\n");
@@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
 			  LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
 			  chip->leds[led].led_current);
 
-		INIT_WORK(&(chip->leds[led].brightness_work),
-			lp5523_led_brightness_work);
-
 		led++;
 	}
 

[patch -resend] leds-lp5523: BUG() in error handling in probe()

[Dan Carpenter]

Inside the error handling in lp5523_init_led(), there is a place that
calls to led_classdev_unregister().  When we unregister the LED drivers,
it tries to set the brightness to OFF.  In this driver setting the
brightness is done through a work queue and the work queue hasn't been
initialized yet.

The result is that we trigger a WARN_ON() in the __queue_work().

The fix is to move the INIT_WORK() in front of the call to
lp5523_init_led().

Matt Renzelmann found this using a bug finding tool.

Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I don't have this hardware, so I can't test it.  I originally sent this
on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
a BUG_ON() so I've updated the commit message.

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 857a3e1..e8a2712 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
 		if (pdata->led_config[i].led_current = 0)
 			continue;
 
+		INIT_WORK(&chip->leds[led].brightness_work,
+			lp5523_led_brightness_work);
+
 		ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
 		if (ret) {
 			dev_err(&client->dev, "error initializing leds\n");
@@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
 			  LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
 			  chip->leds[led].led_current);
 
-		INIT_WORK(&(chip->leds[led].brightness_work),
-			lp5523_led_brightness_work);
-
 		led++;
 	}
 

[patch -resend] Input: ff-memless - fix a couple min_t() casts

[Dan Carpenter]

envelope->attack_level is a u16 type.  We're trying to clamp it here
so it's between 0 and 0x7fff.  Unfortunately, the cast to __s16 turns
all the values larger than 0x7fff into negative numbers and min_t()
thinks they are less than 0x7fff.  envelope_level is an int so now
we've got negative values stored there.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Mon, 26 Sep 2011.  I have added Anssi Hannula to the
CC list.

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 5f55885..b107922 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -176,7 +176,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
 			 value, envelope->attack_level);
 		time_from_level = jiffies_to_msecs(now - state->play_at);
 		time_of_envelope = envelope->attack_length;
-		envelope_level = min_t(__s16, envelope->attack_level, 0x7fff);
+		envelope_level = min_t(u16, envelope->attack_level, 0x7fff);
 
 	} else if (envelope->fade_length && effect->replay.length &&
 		   time_after(now,
@@ -184,7 +184,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
 		   time_before(now, state->stop_at)) {
 		time_from_level = jiffies_to_msecs(state->stop_at - now);
 		time_of_envelope = envelope->fade_length;
-		envelope_level = min_t(__s16, envelope->fade_level, 0x7fff);
+		envelope_level = min_t(u16, envelope->fade_level, 0x7fff);
 	} else
 		return value;
 

[patch -resend] Input: ff-memless - fix a couple min_t() casts

[Dan Carpenter]

envelope->attack_level is a u16 type.  We're trying to clamp it here
so it's between 0 and 0x7fff.  Unfortunately, the cast to __s16 turns
all the values larger than 0x7fff into negative numbers and min_t()
thinks they are less than 0x7fff.  envelope_level is an int so now
we've got negative values stored there.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Mon, 26 Sep 2011.  I have added Anssi Hannula to the
CC list.

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 5f55885..b107922 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -176,7 +176,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
 			 value, envelope->attack_level);
 		time_from_level = jiffies_to_msecs(now - state->play_at);
 		time_of_envelope = envelope->attack_length;
-		envelope_level = min_t(__s16, envelope->attack_level, 0x7fff);
+		envelope_level = min_t(u16, envelope->attack_level, 0x7fff);
 
 	} else if (envelope->fade_length && effect->replay.length &&
 		   time_after(now,
@@ -184,7 +184,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
 		   time_before(now, state->stop_at)) {
 		time_from_level = jiffies_to_msecs(state->stop_at - now);
 		time_of_envelope = envelope->fade_length;
-		envelope_level = min_t(__s16, envelope->fade_level, 0x7fff);
+		envelope_level = min_t(u16, envelope->fade_level, 0x7fff);
 	} else
 		return value;
 

[patch -resend] [patch] tlb_uv: remove some dead code in parse_tunables_write()

[Dan Carpenter]

This code isn't reachable anymore after f073cc8f39 "x86, UV: Clean up
uv_tlb.c" because we will always hit a continue statement.  This causes
a Smatch message:

arch/x86/platform/uv/tlb_uv.c:1531 parse_tunables_write()
	info: ignoring unreachable code.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have cleaned up the commit message.  This was originally sent on
Sun, 7 Aug 2011.

diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index 59880af..d625792 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -1528,8 +1528,6 @@ static int parse_tunables_write(struct bau_control *bcp, char *instr,
 				*tunables[cnt].tunp = val;
 			continue;
 		}
-		if (q == p)
-			break;
 	}
 	return 0;
 }

[patch -resend] [patch] tlb_uv: remove some dead code in parse_tunables_write()

[Dan Carpenter]

This code isn't reachable anymore after f073cc8f39 "x86, UV: Clean up
uv_tlb.c" because we will always hit a continue statement.  This causes
a Smatch message:

arch/x86/platform/uv/tlb_uv.c:1531 parse_tunables_write()
	info: ignoring unreachable code.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have cleaned up the commit message.  This was originally sent on
Sun, 7 Aug 2011.

diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index 59880af..d625792 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -1528,8 +1528,6 @@ static int parse_tunables_write(struct bau_control *bcp, char *instr,
 				*tunables[cnt].tunp = val;
 			continue;
 		}
-		if (q = p)
-			break;
 	}
 	return 0;
 }

Re: [patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[walter harms]

Am 27.06.2012 11:00, schrieb Dan Carpenter:
> scsi_dma_map() returns -ENOMEM on error.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Originally sent on Tue, 20 Sep 2011.
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 374c4ed..b2c3a1a 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
>  
>  	/* Build ASC_SCSI_Q */
>  	use_sg = scsi_dma_map(scp);
> +	if (use_sg < 0) {
> +		scsi_dma_unmap(scp);
> +		scp->result = HOST_BYTE(DID_SOFT_ERROR);
> +		return ASC_ERROR;
> +	}
> +
>  	if (use_sg != 0) {
>  		int sgcnt;
>  		struct scatterlist *slp;

Q:if use_sg = 0 a special case ?

re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[walter harms]

Am 27.06.2012 11:00, schrieb Dan Carpenter:
> scsi_dma_map() returns -ENOMEM on error.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Originally sent on Tue, 20 Sep 2011.
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 374c4ed..b2c3a1a 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
>  
>  	/* Build ASC_SCSI_Q */
>  	use_sg = scsi_dma_map(scp);
> +	if (use_sg < 0) {
> +		scsi_dma_unmap(scp);
> +		scp->result = HOST_BYTE(DID_SOFT_ERROR);
> +		return ASC_ERROR;
> +	}
> +
>  	if (use_sg != 0) {
>  		int sgcnt;
>  		struct scatterlist *slp;

Q:if use_sg == 0 a special case ?

re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 12:01:01PM +0200, walter harms wrote:
> 
> 
> Am 27.06.2012 11:00, schrieb Dan Carpenter:
> > scsi_dma_map() returns -ENOMEM on error.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Originally sent on Tue, 20 Sep 2011.
> > 
> > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> > index 374c4ed..b2c3a1a 100644
> > --- a/drivers/scsi/advansys.c
> > +++ b/drivers/scsi/advansys.c
> > @@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
> >  
> >  	/* Build ASC_SCSI_Q */
> >  	use_sg = scsi_dma_map(scp);
> > +	if (use_sg < 0) {
> > +		scsi_dma_unmap(scp);
> > +		scp->result = HOST_BYTE(DID_SOFT_ERROR);
> > +		return ASC_ERROR;
> > +	}
> > +
> >  	if (use_sg != 0) {
> >  		int sgcnt;
> >  		struct scatterlist *slp;
> 
> Q:if use_sg = 0 a special case ?
> 

Yes.  scsi_dma_map() returns the number of sg lists actually used,
zero if the sg lists is NULL, or -ENOMEM if the mapping failed.

regards,
dan carpenter

Re: [patch 1/2 -resend] SCSI: advansys: handle errors from scsi_dma_map()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 12:01:01PM +0200, walter harms wrote:
> 
> 
> Am 27.06.2012 11:00, schrieb Dan Carpenter:
> > scsi_dma_map() returns -ENOMEM on error.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Originally sent on Tue, 20 Sep 2011.
> > 
> > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> > index 374c4ed..b2c3a1a 100644
> > --- a/drivers/scsi/advansys.c
> > +++ b/drivers/scsi/advansys.c
> > @@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
> >  
> >  	/* Build ASC_SCSI_Q */
> >  	use_sg = scsi_dma_map(scp);
> > +	if (use_sg < 0) {
> > +		scsi_dma_unmap(scp);
> > +		scp->result = HOST_BYTE(DID_SOFT_ERROR);
> > +		return ASC_ERROR;
> > +	}
> > +
> >  	if (use_sg != 0) {
> >  		int sgcnt;
> >  		struct scatterlist *slp;
> 
> Q:if use_sg == 0 a special case ?
> 

Yes.  scsi_dma_map() returns the number of sg lists actually used,
zero if the sg lists is NULL, or -ENOMEM if the mapping failed.

regards,
dan carpenter

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[walter harms]

Am 27.06.2012 11:01, schrieb Dan Carpenter:
> I don't think we're actually likely to hit this limit but if we do
> then the comparison should be done as size_t.  The original code
> is equivalent to:
>         len = strlen(sptr) % USHRT_MAX;
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I was told this patch "has already made it upstream via the v9fs pull."
> but it must have been dropped accidentally.  Originally sent on Sat,
> Jan 15, 2011.
> 
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 9ee48cb..3d33ecf 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>  				const char *sptr = va_arg(ap, const char *);
>  				uint16_t len = 0;
>  				if (sptr)
> -					len = min_t(uint16_t, strlen(sptr),
> +					len = min_t(size_t, strlen(sptr),
>  								USHRT_MAX);
>  
>  				errcode = p9pdu_writef(pdu, proto_version,

this will result in
	uint16_t = size_t
i would expect compilers to complains since uint16 < size_t (most times). In this special case
it seems more easy  write it. also ushort seems ambitious since  uint16_t  need not to be ushort.
so my idea would look like this:

	len=strlen
	if (len>65535) len=65535;
	p9pdu_writef(...,(unint16_t)len);

just my 2 cents,
 wh	
	

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[walter harms]

Am 27.06.2012 11:01, schrieb Dan Carpenter:
> I don't think we're actually likely to hit this limit but if we do
> then the comparison should be done as size_t.  The original code
> is equivalent to:
>         len = strlen(sptr) % USHRT_MAX;
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I was told this patch "has already made it upstream via the v9fs pull."
> but it must have been dropped accidentally.  Originally sent on Sat,
> Jan 15, 2011.
> 
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 9ee48cb..3d33ecf 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>  				const char *sptr = va_arg(ap, const char *);
>  				uint16_t len = 0;
>  				if (sptr)
> -					len = min_t(uint16_t, strlen(sptr),
> +					len = min_t(size_t, strlen(sptr),
>  								USHRT_MAX);
>  
>  				errcode = p9pdu_writef(pdu, proto_version,

this will result in
	uint16_t = size_t
i would expect compilers to complains since uint16 < size_t (most times). In this special case
it seems more easy  write it. also ushort seems ambitious since  uint16_t  need not to be ushort.
so my idea would look like this:

	len=strlen
	if (len>65535) lene535;
	p9pdu_writef(...,(unint16_t)len);

just my 2 cents,
 wh	
	

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 12:19:26PM +0200, walter harms wrote:
> 
> 
> Am 27.06.2012 11:01, schrieb Dan Carpenter:
> > I don't think we're actually likely to hit this limit but if we do
> > then the comparison should be done as size_t.  The original code
> > is equivalent to:
> >         len = strlen(sptr) % USHRT_MAX;
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I was told this patch "has already made it upstream via the v9fs pull."
> > but it must have been dropped accidentally.  Originally sent on Sat,
> > Jan 15, 2011.
> > 
> > diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> > index 9ee48cb..3d33ecf 100644
> > --- a/net/9p/protocol.c
> > +++ b/net/9p/protocol.c
> > @@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
> >  				const char *sptr = va_arg(ap, const char *);
> >  				uint16_t len = 0;
> >  				if (sptr)
> > -					len = min_t(uint16_t, strlen(sptr),
> > +					len = min_t(size_t, strlen(sptr),
> >  								USHRT_MAX);
> >  
> >  				errcode = p9pdu_writef(pdu, proto_version,
> 
> this will result in
> 	uint16_t = size_t
> i would expect compilers to complains since uint16 < size_t
> (most times). In this special case it seems more easy  write it.
> also ushort seems ambitious since  uint16_t  need not to be
> ushort.  so my idea would look like this:
> 
> 	len=strlen
> 	if (len>65535) len=65535;
> 	p9pdu_writef(...,(unint16_t)len);
> 

No.  I'm sorry, what you're saying is complete nonsense.  The whole
point of min_t() is that you can cast to both sides to what you want
before you do the compare.

Obviously I wouldn't submit a patch that introduces a compile
warning.  :/

regards,
dan carpenter

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 12:19:26PM +0200, walter harms wrote:
> 
> 
> Am 27.06.2012 11:01, schrieb Dan Carpenter:
> > I don't think we're actually likely to hit this limit but if we do
> > then the comparison should be done as size_t.  The original code
> > is equivalent to:
> >         len = strlen(sptr) % USHRT_MAX;
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I was told this patch "has already made it upstream via the v9fs pull."
> > but it must have been dropped accidentally.  Originally sent on Sat,
> > Jan 15, 2011.
> > 
> > diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> > index 9ee48cb..3d33ecf 100644
> > --- a/net/9p/protocol.c
> > +++ b/net/9p/protocol.c
> > @@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
> >  				const char *sptr = va_arg(ap, const char *);
> >  				uint16_t len = 0;
> >  				if (sptr)
> > -					len = min_t(uint16_t, strlen(sptr),
> > +					len = min_t(size_t, strlen(sptr),
> >  								USHRT_MAX);
> >  
> >  				errcode = p9pdu_writef(pdu, proto_version,
> 
> this will result in
> 	uint16_t = size_t
> i would expect compilers to complains since uint16 < size_t
> (most times). In this special case it seems more easy  write it.
> also ushort seems ambitious since  uint16_t  need not to be
> ushort.  so my idea would look like this:
> 
> 	len=strlen
> 	if (len>65535) lene535;
> 	p9pdu_writef(...,(unint16_t)len);
> 

No.  I'm sorry, what you're saying is complete nonsense.  The whole
point of min_t() is that you can cast to both sides to what you want
before you do the compare.

Obviously I wouldn't submit a patch that introduces a compile
warning.  :/

regards,
dan carpenter

Re: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Bryan Wu]

On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Inside the error handling in lp5523_init_led(), there is a place that
> calls to led_classdev_unregister().  When we unregister the LED drivers,
> it tries to set the brightness to OFF.  In this driver setting the
> brightness is done through a work queue and the work queue hasn't been
> initialized yet.
>
> The result is that we trigger a WARN_ON() in the __queue_work().
>
> The fix is to move the INIT_WORK() in front of the call to
> lp5523_init_led().
>

Thanks for resending this, I applied this in my for-next branch.

> Matt Renzelmann found this using a bug finding tool.

Just be curious, what's kind of the tool here?

-Bryan

>
> Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I don't have this hardware, so I can't test it.  I originally sent this
> on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
> a BUG_ON() so I've updated the commit message.
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 857a3e1..e8a2712 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
>                if (pdata->led_config[i].led_current == 0)
>                        continue;
>
> +               INIT_WORK(&chip->leds[led].brightness_work,
> +                       lp5523_led_brightness_work);
> +
>                ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
>                if (ret) {
>                        dev_err(&client->dev, "error initializing leds\n");
> @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
>                          LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
>                          chip->leds[led].led_current);
>
> -               INIT_WORK(&(chip->leds[led].brightness_work),
> -                       lp5523_led_brightness_work);
> -
>                led++;
>        }
>



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Bryan Wu]

On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Inside the error handling in lp5523_init_led(), there is a place that
> calls to led_classdev_unregister().  When we unregister the LED drivers,
> it tries to set the brightness to OFF.  In this driver setting the
> brightness is done through a work queue and the work queue hasn't been
> initialized yet.
>
> The result is that we trigger a WARN_ON() in the __queue_work().
>
> The fix is to move the INIT_WORK() in front of the call to
> lp5523_init_led().
>

Thanks for resending this, I applied this in my for-next branch.

> Matt Renzelmann found this using a bug finding tool.

Just be curious, what's kind of the tool here?

-Bryan

>
> Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I don't have this hardware, so I can't test it.  I originally sent this
> on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
> a BUG_ON() so I've updated the commit message.
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 857a3e1..e8a2712 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
>                if (pdata->led_config[i].led_current = 0)
>                        continue;
>
> +               INIT_WORK(&chip->leds[led].brightness_work,
> +                       lp5523_led_brightness_work);
> +
>                ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
>                if (ret) {
>                        dev_err(&client->dev, "error initializing leds\n");
> @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
>                          LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
>                          chip->leds[led].led_current);
>
> -               INIT_WORK(&(chip->leds[led].brightness_work),
> -                       lp5523_led_brightness_work);
> -
>                led++;
>        }
>



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 06:49:10PM +0800, Bryan Wu wrote:
> On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Inside the error handling in lp5523_init_led(), there is a place that
> > calls to led_classdev_unregister().  When we unregister the LED drivers,
> > it tries to set the brightness to OFF.  In this driver setting the
> > brightness is done through a work queue and the work queue hasn't been
> > initialized yet.
> >
> > The result is that we trigger a WARN_ON() in the __queue_work().
> >
> > The fix is to move the INIT_WORK() in front of the call to
> > lp5523_init_led().
> >
> 
> Thanks for resending this, I applied this in my for-next branch.
> 
> > Matt Renzelmann found this using a bug finding tool.
> 
> Just be curious, what's kind of the tool here?
> 

Sorry I should have CC'd Matt on this.  I think it's something he
is working on at the University of Wisconsin.  That's all I know.

regards,
dan carpenter

> -Bryan
> 
> >
> > Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I don't have this hardware, so I can't test it.  I originally sent this
> > on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> > subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
> > a BUG_ON() so I've updated the commit message.
> >
> > diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> > index 857a3e1..e8a2712 100644
> > --- a/drivers/leds/leds-lp5523.c
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> >                if (pdata->led_config[i].led_current == 0)
> >                        continue;
> >
> > +               INIT_WORK(&chip->leds[led].brightness_work,
> > +                       lp5523_led_brightness_work);
> > +
> >                ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
> >                if (ret) {
> >                        dev_err(&client->dev, "error initializing leds\n");
> > @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> >                          LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
> >                          chip->leds[led].led_current);
> >
> > -               INIT_WORK(&(chip->leds[led].brightness_work),
> > -                       lp5523_led_brightness_work);
> > -
> >                led++;
> >        }
> >
> 
> 
> 
> -- 
> Bryan Wu <bryan.wu@canonical.com>
> Kernel Developer    +86.186-168-78255 Mobile
> Canonical Ltd.      www.canonical.com
> Ubuntu - Linux for human beings | www.ubuntu.com

Re: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 06:49:10PM +0800, Bryan Wu wrote:
> On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Inside the error handling in lp5523_init_led(), there is a place that
> > calls to led_classdev_unregister().  When we unregister the LED drivers,
> > it tries to set the brightness to OFF.  In this driver setting the
> > brightness is done through a work queue and the work queue hasn't been
> > initialized yet.
> >
> > The result is that we trigger a WARN_ON() in the __queue_work().
> >
> > The fix is to move the INIT_WORK() in front of the call to
> > lp5523_init_led().
> >
> 
> Thanks for resending this, I applied this in my for-next branch.
> 
> > Matt Renzelmann found this using a bug finding tool.
> 
> Just be curious, what's kind of the tool here?
> 

Sorry I should have CC'd Matt on this.  I think it's something he
is working on at the University of Wisconsin.  That's all I know.

regards,
dan carpenter

> -Bryan
> 
> >
> > Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I don't have this hardware, so I can't test it.  I originally sent this
> > on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> > subsystem.  Also when I sent it, the WARN_ON() in __queue_work() was
> > a BUG_ON() so I've updated the commit message.
> >
> > diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> > index 857a3e1..e8a2712 100644
> > --- a/drivers/leds/leds-lp5523.c
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> >                if (pdata->led_config[i].led_current = 0)
> >                        continue;
> >
> > +               INIT_WORK(&chip->leds[led].brightness_work,
> > +                       lp5523_led_brightness_work);
> > +
> >                ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
> >                if (ret) {
> >                        dev_err(&client->dev, "error initializing leds\n");
> > @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> >                          LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
> >                          chip->leds[led].led_current);
> >
> > -               INIT_WORK(&(chip->leds[led].brightness_work),
> > -                       lp5523_led_brightness_work);
> > -
> >                led++;
> >        }
> >
> 
> 
> 
> -- 
> Bryan Wu <bryan.wu@canonical.com>
> Kernel Developer    +86.186-168-78255 Mobile
> Canonical Ltd.      www.canonical.com
> Ubuntu - Linux for human beings | www.ubuntu.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[walter harms]

Am 27.06.2012 12:36, schrieb Dan Carpenter:
> On Wed, Jun 27, 2012 at 12:19:26PM +0200, walter harms wrote:
>>
>>
>> Am 27.06.2012 11:01, schrieb Dan Carpenter:
>>> I don't think we're actually likely to hit this limit but if we do
>>> then the comparison should be done as size_t.  The original code
>>> is equivalent to:
>>>         len = strlen(sptr) % USHRT_MAX;
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>> I was told this patch "has already made it upstream via the v9fs pull."
>>> but it must have been dropped accidentally.  Originally sent on Sat,
>>> Jan 15, 2011.
>>>
>>> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
>>> index 9ee48cb..3d33ecf 100644
>>> --- a/net/9p/protocol.c
>>> +++ b/net/9p/protocol.c
>>> @@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>>>  				const char *sptr = va_arg(ap, const char *);
>>>  				uint16_t len = 0;
>>>  				if (sptr)
>>> -					len = min_t(uint16_t, strlen(sptr),
>>> +					len = min_t(size_t, strlen(sptr),
>>>  								USHRT_MAX);
>>>  
>>>  				errcode = p9pdu_writef(pdu, proto_version,
>>
>> this will result in
>> 	uint16_t = size_t
>> i would expect compilers to complains since uint16 < size_t
>> (most times). In this special case it seems more easy  write it.
>> also ushort seems ambitious since  uint16_t  need not to be
>> ushort.  so my idea would look like this:
>>
>> 	len=strlen
>> 	if (len>65535) lene535;
>> 	p9pdu_writef(...,(unint16_t)len);
>>
> 
> No.  I'm sorry, what you're saying is complete nonsense.  The whole
> point of min_t() is that you can cast to both sides to what you want
> before you do the compare.
> 
> Obviously I wouldn't submit a patch that introduces a compile
> warning.  :/
> 

i figured it out (i hope) uint16_t is internally long unsigned int not short
as i expected. sorry for the noise.

re,
 wh

Re: [patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Neil Horman]

On Wed, Jun 27, 2012 at 12:08:55PM +0300, Dan Carpenter wrote:
> Even though it has "bool" in the name, you have pass a u32 pointer to
> debugfs_create_bool().  Otherwise you get memory corruption in
> write_file_bool().  Fortunately in this case the corruption happens in
> an alignment hole between variables so it doesn't cause any problems.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
> This was sent on Fri, 2 Mar 2012.
> 
> It's probably my fault the bug is still there.  I submitted the patch
> but the feed back was that debugfs_create_bool() has a stupid API and
> should take a pointer to a bool.  I said that yes, probably that is true
> but only two places use the current API incorrectly and I fixed the
> other place as well.
> 
> I assumed that both my patches would be merged and the person who cared
> about API would fix the API but neither of my fixes were applied.
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 518aea7..66ce414 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
>  static DEFINE_SPINLOCK(free_entries_lock);
>  
>  /* Global disable flag - will be set in case of an error */
> -static bool global_disable __read_mostly;
> +static u32 global_disable __read_mostly;
>  
>  /* Global error count */
>  static u32 error_count;
> @@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
>  
>  	global_disable_dent = debugfs_create_bool("disabled", 0444,
>  			dma_debug_dent,
> -			(u32 *)&global_disable);
> +			&global_disable);
>  	if (!global_disable_dent)
>  		goto out_err;
>  
> 

Re: [patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Neil Horman]

On Wed, Jun 27, 2012 at 12:08:55PM +0300, Dan Carpenter wrote:
> Even though it has "bool" in the name, you have pass a u32 pointer to
> debugfs_create_bool().  Otherwise you get memory corruption in
> write_file_bool().  Fortunately in this case the corruption happens in
> an alignment hole between variables so it doesn't cause any problems.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
> This was sent on Fri, 2 Mar 2012.
> 
> It's probably my fault the bug is still there.  I submitted the patch
> but the feed back was that debugfs_create_bool() has a stupid API and
> should take a pointer to a bool.  I said that yes, probably that is true
> but only two places use the current API incorrectly and I fixed the
> other place as well.
> 
> I assumed that both my patches would be merged and the person who cared
> about API would fix the API but neither of my fixes were applied.
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 518aea7..66ce414 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
>  static DEFINE_SPINLOCK(free_entries_lock);
>  
>  /* Global disable flag - will be set in case of an error */
> -static bool global_disable __read_mostly;
> +static u32 global_disable __read_mostly;
>  
>  /* Global error count */
>  static u32 error_count;
> @@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
>  
>  	global_disable_dent = debugfs_create_bool("disabled", 0444,
>  			dma_debug_dent,
> -			(u32 *)&global_disable);
> +			&global_disable);
>  	if (!global_disable_dent)
>  		goto out_err;
>  
> 

Re: [patch v3 -resend] edac i5000, i5400: fix pointer math in i5000_get_mc_regs()

[Mauro Carvalho Chehab]

Em 27-06-2012 06:07, Dan Carpenter escreveu:
> "pvt->ambase" is a u64 datatype.  The intent here is to fill the first
> half in the first call to pci_read_config_dword() and the other half in
> the second.  Unfortunately the pointer math is wrong so we set the wrong
> data.

Applied, thanks!
Mauro

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Redid it as with a union as Walter Harms suggested.
>      Fixed the same bug in i5400_edac.c as well.
> v3: Make the struct __packed just in case.
> 
> I don't have this hardware, so please review carefully.  But the
> original code obviously corrupts memory so my patch could hardly be
> worse...
> 
> This was originally sent on Mon, Mar 5, 2012.  Btw, there are some
> Sparse errors in these files.  The code looks buggy but I don't know
> what the intent was.
> 
> drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
> drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value
> 
> diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
> index a5c33df..39c6375 100644
> --- a/drivers/edac/i5000_edac.c
> +++ b/drivers/edac/i5000_edac.c
> @@ -328,7 +328,13 @@ struct i5000_pvt {
>   	struct pci_dev *branch_1;	/* 22.0 */
>   
>   	u16 tolm;		/* top of low memory */
> -	u64 ambase;		/* AMB BAR */
> +	union {
> +		u64 ambase;		/* AMB BAR */
> +		struct {
> +			u32 ambase_bottom;
> +			u32 ambase_top;
> +		} u __packed;
> +	};
>   
>   	u16 mir0, mir1, mir2;
>   
> @@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
>   	pvt = mci->pvt_info;
>   
>   	pci_read_config_dword(pvt->system_address, AMBASE,
> -			(u32 *) & pvt->ambase);
> +			&pvt->u.ambase_bottom);
>   	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> -			((u32 *) & pvt->ambase) + sizeof(u32));
> +			&pvt->u.ambase_top);
>   
>   	maxdimmperch = pvt->maxdimmperch;
>   	maxch = pvt->maxch;
> diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
> index 50069c6..2772469 100644
> --- a/drivers/edac/i5400_edac.c
> +++ b/drivers/edac/i5400_edac.c
> @@ -327,7 +327,13 @@ struct i5400_pvt {
>   	struct pci_dev *branch_1;		/* 22.0 */
>   
>   	u16 tolm;				/* top of low memory */
> -	u64 ambase;				/* AMB BAR */
> +	union {
> +		u64 ambase;				/* AMB BAR */
> +		struct {
> +			u32 ambase_bottom;
> +			u32 ambase_top;
> +		} u __packed;
> +	};
>   
>   	u16 mir0, mir1;
>   
> @@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
>   	pvt = mci->pvt_info;
>   
>   	pci_read_config_dword(pvt->system_address, AMBASE,
> -			(u32 *) &pvt->ambase);
> +			&pvt->u.ambase_bottom);
>   	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> -			((u32 *) &pvt->ambase) + sizeof(u32));
> +			&pvt->u.ambase_top);
>   
>   	maxdimmperch = pvt->maxdimmperch;
>   	maxch = pvt->maxch;
> 

Re: [patch v3 -resend] edac i5000, i5400: fix pointer math in i5000_get_mc_regs()

[Mauro Carvalho Chehab]

Em 27-06-2012 06:07, Dan Carpenter escreveu:
> "pvt->ambase" is a u64 datatype.  The intent here is to fill the first
> half in the first call to pci_read_config_dword() and the other half in
> the second.  Unfortunately the pointer math is wrong so we set the wrong
> data.

Applied, thanks!
Mauro

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Redid it as with a union as Walter Harms suggested.
>      Fixed the same bug in i5400_edac.c as well.
> v3: Make the struct __packed just in case.
> 
> I don't have this hardware, so please review carefully.  But the
> original code obviously corrupts memory so my patch could hardly be
> worse...
> 
> This was originally sent on Mon, Mar 5, 2012.  Btw, there are some
> Sparse errors in these files.  The code looks buggy but I don't know
> what the intent was.
> 
> drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
> drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value
> 
> diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
> index a5c33df..39c6375 100644
> --- a/drivers/edac/i5000_edac.c
> +++ b/drivers/edac/i5000_edac.c
> @@ -328,7 +328,13 @@ struct i5000_pvt {
>   	struct pci_dev *branch_1;	/* 22.0 */
>   
>   	u16 tolm;		/* top of low memory */
> -	u64 ambase;		/* AMB BAR */
> +	union {
> +		u64 ambase;		/* AMB BAR */
> +		struct {
> +			u32 ambase_bottom;
> +			u32 ambase_top;
> +		} u __packed;
> +	};
>   
>   	u16 mir0, mir1, mir2;
>   
> @@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
>   	pvt = mci->pvt_info;
>   
>   	pci_read_config_dword(pvt->system_address, AMBASE,
> -			(u32 *) & pvt->ambase);
> +			&pvt->u.ambase_bottom);
>   	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> -			((u32 *) & pvt->ambase) + sizeof(u32));
> +			&pvt->u.ambase_top);
>   
>   	maxdimmperch = pvt->maxdimmperch;
>   	maxch = pvt->maxch;
> diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
> index 50069c6..2772469 100644
> --- a/drivers/edac/i5400_edac.c
> +++ b/drivers/edac/i5400_edac.c
> @@ -327,7 +327,13 @@ struct i5400_pvt {
>   	struct pci_dev *branch_1;		/* 22.0 */
>   
>   	u16 tolm;				/* top of low memory */
> -	u64 ambase;				/* AMB BAR */
> +	union {
> +		u64 ambase;				/* AMB BAR */
> +		struct {
> +			u32 ambase_bottom;
> +			u32 ambase_top;
> +		} u __packed;
> +	};
>   
>   	u16 mir0, mir1;
>   
> @@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
>   	pvt = mci->pvt_info;
>   
>   	pci_read_config_dword(pvt->system_address, AMBASE,
> -			(u32 *) &pvt->ambase);
> +			&pvt->u.ambase_bottom);
>   	pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> -			((u32 *) &pvt->ambase) + sizeof(u32));
> +			&pvt->u.ambase_top);
>   
>   	maxdimmperch = pvt->maxdimmperch;
>   	maxch = pvt->maxch;
> 

Re: [patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Mauro Carvalho Chehab]

Em 27-06-2012 06:06, Dan Carpenter escreveu:
> The intent here was to test that the flag was clear but the '!' has
> higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
> equivalent to "&& (!sgs[i].flags) ..."
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> fix on Thu, May 3, 2012.
> 
> diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> index 4008b9c..f6f0cf9 100644
> --- a/drivers/media/dvb/dvb-usb/az6007.c
> +++ b/drivers/media/dvb/dvb-usb/az6007.c
> @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   		addr = msgs[i].addr << 1;
>   		if (((i + 1) < num)
>   		    && (msgs[i].len == 1)
> -		    && (!msgs[i].flags & I2C_M_RD)
> +		    && (!(msgs[i].flags & I2C_M_RD))
>   		    && (msgs[i + 1].flags & I2C_M_RD)
>   		    && (msgs[i].addr == msgs[i + 1].addr)) {
>   			/*
> 

Dan,

Your logic is correct, however, I didn't apply this patch because it broke
the driver.

I'll need to re-visit the driver when I have some time, in order to be
able to apply this one, without breaking the driver. I'll likely need to
change some other things on this routine.

(this has a low priority, as the driver is working properly the way it is).

So, I'm keeping your patch at patchwork, while I don't find some time for it.

Regards,
Mauro

Re: [patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Mauro Carvalho Chehab]

Em 27-06-2012 06:06, Dan Carpenter escreveu:
> The intent here was to test that the flag was clear but the '!' has
> higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
> equivalent to "&& (!sgs[i].flags) ..."
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> fix on Thu, May 3, 2012.
> 
> diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> index 4008b9c..f6f0cf9 100644
> --- a/drivers/media/dvb/dvb-usb/az6007.c
> +++ b/drivers/media/dvb/dvb-usb/az6007.c
> @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   		addr = msgs[i].addr << 1;
>   		if (((i + 1) < num)
>   		    && (msgs[i].len = 1)
> -		    && (!msgs[i].flags & I2C_M_RD)
> +		    && (!(msgs[i].flags & I2C_M_RD))
>   		    && (msgs[i + 1].flags & I2C_M_RD)
>   		    && (msgs[i].addr = msgs[i + 1].addr)) {
>   			/*
> 

Dan,

Your logic is correct, however, I didn't apply this patch because it broke
the driver.

I'll need to re-visit the driver when I have some time, in order to be
able to apply this one, without breaking the driver. I'll likely need to
change some other things on this routine.

(this has a low priority, as the driver is working properly the way it is).

So, I'm keeping your patch at patchwork, while I don't find some time for it.

Regards,
Mauro

RE: [patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

[Krishna Gudipati]

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com] 
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Subject: [patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

If mc == BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Wed, 6 Jul 2011.

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
 			return;
 		}
 
-		if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
+		if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
 			return;
 
 		mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);


-----

Thanks for the patch.

Acked-by: Krishna Gudipati <kgudipat@brocade.com>

RE: [patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

[Krishna Gudipati]

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com] 
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Subject: [patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()

If mc = BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Originally sent on Wed, 6 Jul 2011.

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
 			return;
 		}
 
-		if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn = NULL))
+		if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn = NULL))
 			return;
 
 		mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);


-----

Thanks for the patch.

Acked-by: Krishna Gudipati <kgudipat@brocade.com>

RE: [patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

[Krishna Gudipati]

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com] 
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Subject: [patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.

I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto.  I looked at other functions in
this file and it seems like returning directly is standard.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is the third time I have sent this patch.  It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.

diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
 bfad_im_probe(struct bfad_s *bfad)
 {
 	struct bfad_im_s      *im;
-	bfa_status_t    rc = BFA_STATUS_OK;
 
 	im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
-	if (im == NULL) {
-		rc = BFA_STATUS_ENOMEM;
-		goto ext;
-	}
+	if (im == NULL)
+		return BFA_STATUS_ENOMEM;
 
 	bfad->im = im;
 	im->bfad = bfad;
 
 	if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
 		kfree(im);
-		rc = BFA_STATUS_FAILED;
+		return BFA_STATUS_FAILED;
 	}
 
 	INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
-	return rc;
+	return BFA_STATUS_OK;
 }
 
 void

-----

Thanks for the patch.

Acked-by: Krishna Gudipati <kgudipat@brocade.com>

RE: [patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

[Krishna Gudipati]

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com] 
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Subject: [patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()

If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.

I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto.  I looked at other functions in
this file and it seems like returning directly is standard.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is the third time I have sent this patch.  It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.

diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
 bfad_im_probe(struct bfad_s *bfad)
 {
 	struct bfad_im_s      *im;
-	bfa_status_t    rc = BFA_STATUS_OK;
 
 	im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
-	if (im = NULL) {
-		rc = BFA_STATUS_ENOMEM;
-		goto ext;
-	}
+	if (im = NULL)
+		return BFA_STATUS_ENOMEM;
 
 	bfad->im = im;
 	im->bfad = bfad;
 
 	if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
 		kfree(im);
-		rc = BFA_STATUS_FAILED;
+		return BFA_STATUS_FAILED;
 	}
 
 	INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
-	return rc;
+	return BFA_STATUS_OK;
 }
 
 void

-----

Thanks for the patch.

Acked-by: Krishna Gudipati <kgudipat@brocade.com>

Re: [patch -resend] [SCSI] isci: add a couple __iomem annotations

[Dan Williams]

On Wed, Jun 27, 2012 at 2:05 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> These are __iomem.  Sparse complains if we don't have that.
>
> drivers/scsi/isci/phy.c +149 70: warning:
>        incorrect type in initializer (different address spaces)
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Sent on Wed, 18 Jan 2012.
>

Thanks for nudge, this fell off the radar.  I'll get this into -next
with the rest of the pending bits.

Re: [patch -resend] [SCSI] isci: add a couple __iomem annotations

[Dan Williams]

On Wed, Jun 27, 2012 at 2:05 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> These are __iomem.  Sparse complains if we don't have that.
>
> drivers/scsi/isci/phy.c +149 70: warning:
>        incorrect type in initializer (different address spaces)
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Sent on Wed, 18 Jan 2012.
>

Thanks for nudge, this fell off the radar.  I'll get this into -next
with the rest of the pending bits.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 27 Jun 2012 12:01:41 +0300

> I don't think we're actually likely to hit this limit but if we do
> then the comparison should be done as size_t.  The original code
> is equivalent to:
>         len = strlen(sptr) % USHRT_MAX;
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.

Re: [patch -resend] 9p: fix min_t() casting in p9pdu_vwritef()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 27 Jun 2012 12:01:41 +0300

> I don't think we're actually likely to hit this limit but if we do
> then the comparison should be done as size_t.  The original code
> is equivalent to:
>         len = strlen(sptr) % USHRT_MAX;
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.

Re: [patch -resend] [SCSI] megaraid: cleanup type issue in mega_build_cmd()

[adam radford]

On Wed, Jun 27, 2012 at 2:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On 64 bit systems the current code sets 32 bits of "seg" and leaves the
> other 32 uninitialized.  It doesn't matter since the variable is never
> used.  But it's still messy and we should fix it.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Originally sent on Fri, 2 Mar 2012.
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 4d39a9f..97825f1 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
>        mega_passthru   *pthru;
>        scb_t   *scb;
>        mbox_t  *mbox;
> -       long    seg;
> +       u32     seg;
>        char    islogical;
>        int     max_ldrv_num;
>        int     channel = 0;
> @@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
>
>                        /* Calculate Scatter-Gather info */
>                        mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> -                                       (u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
> +                                       (u32 *)&mbox->m_out.xferaddr, &seg);
>
>                        return scb;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Acked-by: Adam Radford <aradford@gmail.com>

Re: [patch -resend] [SCSI] megaraid: cleanup type issue in mega_build_cmd()

[adam radford]

On Wed, Jun 27, 2012 at 2:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On 64 bit systems the current code sets 32 bits of "seg" and leaves the
> other 32 uninitialized.  It doesn't matter since the variable is never
> used.  But it's still messy and we should fix it.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Originally sent on Fri, 2 Mar 2012.
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 4d39a9f..97825f1 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
>        mega_passthru   *pthru;
>        scb_t   *scb;
>        mbox_t  *mbox;
> -       long    seg;
> +       u32     seg;
>        char    islogical;
>        int     max_ldrv_num;
>        int     channel = 0;
> @@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
>
>                        /* Calculate Scatter-Gather info */
>                        mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> -                                       (u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
> +                                       (u32 *)&mbox->m_out.xferaddr, &seg);
>
>                        return scb;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Acked-by: Adam Radford <aradford@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -resend] [SCSI] megaraid: remove a spurious IRQ enable

[adam radford]

On Wed, Jun 27, 2012 at 2:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We took this lock with spin_lock() so we should unlock it with
> spin_unlock() instead of spin_unlock_irq().  This was introduced in
> f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This was originally sent on Sat, 30 Jul 2011.  I have cleaned up the
> commit message a bit and added Christoph to the CC list.
>
> diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
> index 35bd138..54b1c5b 100644
> --- a/drivers/scsi/megaraid/megaraid_mbox.c
> +++ b/drivers/scsi/megaraid/megaraid_mbox.c
> @@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
>        }
>
>  out:
> -       spin_unlock_irq(&adapter->lock);
> +       spin_unlock(&adapter->lock);
>        return rval;
>  }
>
> --
> 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

Acked-by: Adam Radford <aradford@gmail.com>

Re: [patch -resend] [SCSI] megaraid: remove a spurious IRQ enable

[adam radford]

On Wed, Jun 27, 2012 at 2:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We took this lock with spin_lock() so we should unlock it with
> spin_unlock() instead of spin_unlock_irq().  This was introduced in
> f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This was originally sent on Sat, 30 Jul 2011.  I have cleaned up the
> commit message a bit and added Christoph to the CC list.
>
> diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
> index 35bd138..54b1c5b 100644
> --- a/drivers/scsi/megaraid/megaraid_mbox.c
> +++ b/drivers/scsi/megaraid/megaraid_mbox.c
> @@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
>        }
>
>  out:
> -       spin_unlock_irq(&adapter->lock);
> +       spin_unlock(&adapter->lock);
>        return rval;
>  }
>
> --
> 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

Acked-by: Adam Radford <aradford@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 10:11:00AM -0300, Mauro Carvalho Chehab wrote:
> Em 27-06-2012 06:06, Dan Carpenter escreveu:
> > The intent here was to test that the flag was clear but the '!' has
> > higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
> > equivalent to "&& (!sgs[i].flags) ..."
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> > fix on Thu, May 3, 2012.
> > 
> > diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> > index 4008b9c..f6f0cf9 100644
> > --- a/drivers/media/dvb/dvb-usb/az6007.c
> > +++ b/drivers/media/dvb/dvb-usb/az6007.c
> > @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> >   		addr = msgs[i].addr << 1;
> >   		if (((i + 1) < num)
> >   		    && (msgs[i].len == 1)
> > -		    && (!msgs[i].flags & I2C_M_RD)
> > +		    && (!(msgs[i].flags & I2C_M_RD))
> >   		    && (msgs[i + 1].flags & I2C_M_RD)
> >   		    && (msgs[i].addr == msgs[i + 1].addr)) {
> >   			/*
> > 
> 
> Dan,
> 
> Your logic is correct, however, I didn't apply this patch because it broke
> the driver.
> 
> I'll need to re-visit the driver when I have some time, in order to be
> able to apply this one, without breaking the driver. I'll likely need to
> change some other things on this routine.
> 
> (this has a low priority, as the driver is working properly the way it is).
> 
> So, I'm keeping your patch at patchwork, while I don't find some time for it.

We could just put a comment next to the code and forget about it.

               && (!(msgs[i].flags & I2C_M_RD)) /* the fix needs testing. */

Sparse complains about this so it people are going to keep sending
patches for it.  It's not like you should be stuck doing all the
work.

regards,
dan carpenter

Re: [patch -resend] [media] az6007: precedence bug in az6007_i2c_xfer()

[Dan Carpenter]

On Wed, Jun 27, 2012 at 10:11:00AM -0300, Mauro Carvalho Chehab wrote:
> Em 27-06-2012 06:06, Dan Carpenter escreveu:
> > The intent here was to test that the flag was clear but the '!' has
> > higher precedence than the '&'.  I2C_M_RD is 0x1 so the current code is
> > equivalent to "&& (!sgs[i].flags) ..."
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> > fix on Thu, May 3, 2012.
> > 
> > diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> > index 4008b9c..f6f0cf9 100644
> > --- a/drivers/media/dvb/dvb-usb/az6007.c
> > +++ b/drivers/media/dvb/dvb-usb/az6007.c
> > @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> >   		addr = msgs[i].addr << 1;
> >   		if (((i + 1) < num)
> >   		    && (msgs[i].len = 1)
> > -		    && (!msgs[i].flags & I2C_M_RD)
> > +		    && (!(msgs[i].flags & I2C_M_RD))
> >   		    && (msgs[i + 1].flags & I2C_M_RD)
> >   		    && (msgs[i].addr = msgs[i + 1].addr)) {
> >   			/*
> > 
> 
> Dan,
> 
> Your logic is correct, however, I didn't apply this patch because it broke
> the driver.
> 
> I'll need to re-visit the driver when I have some time, in order to be
> able to apply this one, without breaking the driver. I'll likely need to
> change some other things on this routine.
> 
> (this has a low priority, as the driver is working properly the way it is).
> 
> So, I'm keeping your patch at patchwork, while I don't find some time for it.

We could just put a comment next to the code and forget about it.

               && (!(msgs[i].flags & I2C_M_RD)) /* the fix needs testing. */

Sparse complains about this so it people are going to keep sending
patches for it.  It's not like you should be stuck doing all the
work.

regards,
dan carpenter

RE: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Matt Renzelmann]

> >
> > Just be curious, what's kind of the tool here?
> >
> 
> Sorry I should have CC'd Matt on this.  I think it's something he
> is working on at the University of Wisconsin.  That's all I know.
> 
> regards,
> dan carpenter
> 
> > -Bryan

Dan is correct.  We've submitted the project for publication but whether it will
be accepted is another question.  If it's published I'd be happy to provide a
reference.

In a nutshell, the tool we've developed uses symbolic execution to reduce the
need for device hardware for driver testing and execute real Linux drivers in
the kernel.  It provides symbolic data in place of hardware input.  The tool
allows us to run real drivers even if the hardware is unavailable.  It can't
find all bugs, but it can find many that could manifest in real conditions, e.g.
null pointer dereferences, incorrect kernel API usage, some "panics" and "BUG"
calls, etc.

I'll try to remember to write back when it's published, at which point I'd be
happy to provide more detail.

Thanks for the interest,
Matt

RE: [patch -resend] leds-lp5523: BUG() in error handling in probe()

[Matt Renzelmann]

> >
> > Just be curious, what's kind of the tool here?
> >
> 
> Sorry I should have CC'd Matt on this.  I think it's something he
> is working on at the University of Wisconsin.  That's all I know.
> 
> regards,
> dan carpenter
> 
> > -Bryan

Dan is correct.  We've submitted the project for publication but whether it will
be accepted is another question.  If it's published I'd be happy to provide a
reference.

In a nutshell, the tool we've developed uses symbolic execution to reduce the
need for device hardware for driver testing and execute real Linux drivers in
the kernel.  It provides symbolic data in place of hardware input.  The tool
allows us to run real drivers even if the hardware is unavailable.  It can't
find all bugs, but it can find many that could manifest in real conditions, e.g.
null pointer dereferences, incorrect kernel API usage, some "panics" and "BUG"
calls, etc.

I'll try to remember to write back when it's published, at which point I'd be
happy to provide more detail.

Thanks for the interest,
Matt

Re: [patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Joerg Roedel]

On Wed, Jun 27, 2012 at 12:08:55PM +0300, Dan Carpenter wrote:
> Even though it has "bool" in the name, you have pass a u32 pointer to
> debugfs_create_bool().  Otherwise you get memory corruption in
> write_file_bool().  Fortunately in this case the corruption happens in
> an alignment hole between variables so it doesn't cause any problems.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied both, thanks Dan.

Re: [patch 1/2 -resend] dma-debug: debugfs_create_bool() takes a u32 pointer

[Joerg Roedel]

On Wed, Jun 27, 2012 at 12:08:55PM +0300, Dan Carpenter wrote:
> Even though it has "bool" in the name, you have pass a u32 pointer to
> debugfs_create_bool().  Otherwise you get memory corruption in
> write_file_bool().  Fortunately in this case the corruption happens in
> an alignment hole between variables so it doesn't cause any problems.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied both, thanks Dan.

Re: [patch -resend] Input: ff-memless - fix a couple min_t() casts

[Dmitry Torokhov]

On Wed, Jun 27, 2012 at 12:11:19PM +0300, Dan Carpenter wrote:
> envelope->attack_level is a u16 type.  We're trying to clamp it here
> so it's between 0 and 0x7fff.  Unfortunately, the cast to __s16 turns
> all the values larger than 0x7fff into negative numbers and min_t()
> thinks they are less than 0x7fff.  envelope_level is an int so now
> we've got negative values stored there.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thank you Dan.

> ---
> Originally sent on Mon, 26 Sep 2011.  I have added Anssi Hannula to the
> CC list.
> 
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 5f55885..b107922 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -176,7 +176,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
>  			 value, envelope->attack_level);
>  		time_from_level = jiffies_to_msecs(now - state->play_at);
>  		time_of_envelope = envelope->attack_length;
> -		envelope_level = min_t(__s16, envelope->attack_level, 0x7fff);
> +		envelope_level = min_t(u16, envelope->attack_level, 0x7fff);
>  
>  	} else if (envelope->fade_length && effect->replay.length &&
>  		   time_after(now,
> @@ -184,7 +184,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
>  		   time_before(now, state->stop_at)) {
>  		time_from_level = jiffies_to_msecs(state->stop_at - now);
>  		time_of_envelope = envelope->fade_length;
> -		envelope_level = min_t(__s16, envelope->fade_level, 0x7fff);
> +		envelope_level = min_t(u16, envelope->fade_level, 0x7fff);
>  	} else
>  		return value;
>  

-- 
Dmitry

Re: [patch -resend] Input: ff-memless - fix a couple min_t() casts

[Dmitry Torokhov]

On Wed, Jun 27, 2012 at 12:11:19PM +0300, Dan Carpenter wrote:
> envelope->attack_level is a u16 type.  We're trying to clamp it here
> so it's between 0 and 0x7fff.  Unfortunately, the cast to __s16 turns
> all the values larger than 0x7fff into negative numbers and min_t()
> thinks they are less than 0x7fff.  envelope_level is an int so now
> we've got negative values stored there.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thank you Dan.

> ---
> Originally sent on Mon, 26 Sep 2011.  I have added Anssi Hannula to the
> CC list.
> 
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 5f55885..b107922 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -176,7 +176,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
>  			 value, envelope->attack_level);
>  		time_from_level = jiffies_to_msecs(now - state->play_at);
>  		time_of_envelope = envelope->attack_length;
> -		envelope_level = min_t(__s16, envelope->attack_level, 0x7fff);
> +		envelope_level = min_t(u16, envelope->attack_level, 0x7fff);
>  
>  	} else if (envelope->fade_length && effect->replay.length &&
>  		   time_after(now,
> @@ -184,7 +184,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
>  		   time_before(now, state->stop_at)) {
>  		time_from_level = jiffies_to_msecs(state->stop_at - now);
>  		time_of_envelope = envelope->fade_length;
> -		envelope_level = min_t(__s16, envelope->fade_level, 0x7fff);
> +		envelope_level = min_t(u16, envelope->fade_level, 0x7fff);
>  	} else
>  		return value;
>  

-- 
Dmitry