[char-misc-next 0/3] mei: driver cleanups

[char-misc-next 0/3] mei: driver cleanups

[Tomas Winkler]

*** BLURB HERE ***

Alexander Usyskin (1):
  mei: drop redundant list_del_init

Tomas Winkler (2):
  mei: remove flash_work_queue
  mei: cleanup mei_irq_read_handler

 drivers/misc/mei/client.c    |  2 --
 drivers/misc/mei/init.c      | 17 +++++++---
 drivers/misc/mei/interrupt.c | 79 ++++++++++++++++++++++----------------------
 drivers/misc/mei/mei_dev.h   | 13 +++++++-
 drivers/misc/mei/nfc.c       |  6 +++-
 drivers/misc/mei/pci-me.c    |  2 +-
 6 files changed, 70 insertions(+), 49 deletions(-)

-- 
1.8.3.1

[char-misc-next 1/3] mei: remove flash_work_queue

[Tomas Winkler]

Cancel each work properly and remove flash_work_queue.

Quoting documentation:

In most situations flushing the entire workqueue is overkill; you merely
need to know that a particular work item isn't queued and isn't running.
In such cases you should use cancel_delayed_work_sync() or
cancel_work_sync() instead.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/init.c    | 17 ++++++++++++-----
 drivers/misc/mei/mei_dev.h |  3 ++-
 drivers/misc/mei/nfc.c     |  6 +++++-
 drivers/misc/mei/pci-me.c  |  2 +-
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/mei/init.c b/drivers/misc/mei/init.c
index 7dfc7a4..f83da33 100644
--- a/drivers/misc/mei/init.c
+++ b/drivers/misc/mei/init.c
@@ -131,6 +131,15 @@ err:
 }
 EXPORT_SYMBOL_GPL(mei_start);
 
+
+void mei_cancel_work(struct mei_device *dev)
+{
+	cancel_work_sync(&dev->init_work);
+
+	cancel_delayed_work(&dev->timer_work);
+}
+EXPORT_SYMBOL_GPL(mei_cancel_work);
+
 /**
  * mei_reset - resets host and fw.
  *
@@ -216,16 +225,14 @@ void mei_stop(struct mei_device *dev)
 {
 	dev_dbg(&dev->pdev->dev, "stopping the device.\n");
 
-	flush_scheduled_work();
+	mei_cancel_work(dev);
 
-	mutex_lock(&dev->device_lock);
+	mei_nfc_host_exit(dev);
 
-	cancel_delayed_work(&dev->timer_work);
+	mutex_lock(&dev->device_lock);
 
 	mei_wd_stop(dev);
 
-	mei_nfc_host_exit();
-
 	dev->dev_state = MEI_DEV_POWER_DOWN;
 	mei_reset(dev, 0);
 
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index 406f68e..6f577bf 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -463,6 +463,7 @@ void mei_device_init(struct mei_device *dev);
 void mei_reset(struct mei_device *dev, int interrupts);
 int mei_start(struct mei_device *dev);
 void mei_stop(struct mei_device *dev);
+void mei_cancel_work(struct mei_device *dev);
 
 /*
  *  MEI interrupt functions prototype
@@ -510,7 +511,7 @@ int mei_amthif_irq_read(struct mei_device *dev, s32 *slots);
  * NFC functions
  */
 int mei_nfc_host_init(struct mei_device *dev);
-void mei_nfc_host_exit(void);
+void mei_nfc_host_exit(struct mei_device *dev);
 
 /*
  * NFC Client UUID
diff --git a/drivers/misc/mei/nfc.c b/drivers/misc/mei/nfc.c
index 994ca4a..0a89220 100644
--- a/drivers/misc/mei/nfc.c
+++ b/drivers/misc/mei/nfc.c
@@ -547,12 +547,16 @@ err:
 	return ret;
 }
 
-void mei_nfc_host_exit(void)
+void mei_nfc_host_exit(struct mei_device *dev)
 {
 	struct mei_nfc_dev *ndev = &nfc_dev;
 
+	cancel_work_sync(&ndev->init_work);
+
+	mutex_lock(&dev->device_lock);
 	if (ndev->cl && ndev->cl->device)
 		mei_cl_remove_device(ndev->cl->device);
 
 	mei_nfc_free(ndev);
+	mutex_unlock(&dev->device_lock);
 }
diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index b96205a..3bfae38 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -195,8 +195,8 @@ static int mei_me_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return 0;
 
 release_irq:
+	mei_cancel_work(dev);
 	mei_disable_interrupts(dev);
-	flush_scheduled_work();
 	free_irq(pdev->irq, dev);
 disable_msi:
 	pci_disable_msi(pdev);
-- 
1.8.3.1

[char-misc-next 2/3] mei: drop redundant list_del_init

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

list_del_init appears twice in row in mei_cl_unlink, drop one.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/client.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 87c96e4..6184e85 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -344,8 +344,6 @@ int mei_cl_unlink(struct mei_cl *cl)
 
 	cl->state = MEI_FILE_INITIALIZING;
 
-	list_del_init(&cl->link);
-
 	return 0;
 }
 
-- 
1.8.3.1

[char-misc-next 3/3] mei: cleanup mei_irq_read_handler

[Tomas Winkler]

1. Simplify function flow
2. Display errors in error or warnings level instead of debug.
3. Remove excessive debug messages

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/interrupt.c | 79 ++++++++++++++++++++++----------------------
 drivers/misc/mei/mei_dev.h   | 10 ++++++
 2 files changed, 50 insertions(+), 39 deletions(-)

diff --git a/drivers/misc/mei/interrupt.c b/drivers/misc/mei/interrupt.c
index 7a95c07..9c8225b 100644
--- a/drivers/misc/mei/interrupt.c
+++ b/drivers/misc/mei/interrupt.c
@@ -301,13 +301,11 @@ int mei_irq_read_handler(struct mei_device *dev,
 		struct mei_cl_cb *cmpl_list, s32 *slots)
 {
 	struct mei_msg_hdr *mei_hdr;
-	struct mei_cl *cl_pos = NULL;
-	struct mei_cl *cl_next = NULL;
-	int ret = 0;
+	struct mei_cl *cl;
+	int ret;
 
 	if (!dev->rd_msg_hdr) {
 		dev->rd_msg_hdr = mei_read_hdr(dev);
-		dev_dbg(&dev->pdev->dev, "slots =%08x.\n", *slots);
 		(*slots)--;
 		dev_dbg(&dev->pdev->dev, "slots =%08x.\n", *slots);
 	}
@@ -315,61 +313,64 @@ int mei_irq_read_handler(struct mei_device *dev,
 	dev_dbg(&dev->pdev->dev, MEI_HDR_FMT, MEI_HDR_PRM(mei_hdr));
 
 	if (mei_hdr->reserved || !dev->rd_msg_hdr) {
-		dev_dbg(&dev->pdev->dev, "corrupted message header.\n");
+		dev_err(&dev->pdev->dev, "corrupted message header 0x%08X\n",
+				dev->rd_msg_hdr);
 		ret = -EBADMSG;
 		goto end;
 	}
 
-	if (mei_hdr->host_addr || mei_hdr->me_addr) {
-		list_for_each_entry_safe(cl_pos, cl_next,
-					&dev->file_list, link) {
-			dev_dbg(&dev->pdev->dev,
-					"list_for_each_entry_safe read host"
-					" client = %d, ME client = %d\n",
-					cl_pos->host_client_id,
-					cl_pos->me_client_id);
-			if (mei_cl_hbm_equal(cl_pos, mei_hdr))
-				break;
-		}
-
-		if (&cl_pos->link == &dev->file_list) {
-			dev_dbg(&dev->pdev->dev, "corrupted message header\n");
-			ret = -EBADMSG;
-			goto end;
-		}
-	}
-	if (((*slots) * sizeof(u32)) < mei_hdr->length) {
-		dev_err(&dev->pdev->dev,
-				"we can't read the message slots =%08x.\n",
+	if (mei_slots2data(*slots) < mei_hdr->length) {
+		dev_err(&dev->pdev->dev, "less data available than length=%08x.\n",
 				*slots);
 		/* we can't read the message */
 		ret = -ERANGE;
 		goto end;
 	}
 
-	/* decide where to read the message too */
-	if (!mei_hdr->host_addr) {
-		dev_dbg(&dev->pdev->dev, "call mei_hbm_dispatch.\n");
+	/*  HBM message */
+	if (mei_hdr->host_addr == 0 && mei_hdr->me_addr == 0) {
 		mei_hbm_dispatch(dev, mei_hdr);
-		dev_dbg(&dev->pdev->dev, "end mei_hbm_dispatch.\n");
-	} else if (mei_hdr->host_addr == dev->iamthif_cl.host_client_id &&
-		   (MEI_FILE_CONNECTED == dev->iamthif_cl.state) &&
-		   (dev->iamthif_state == MEI_IAMTHIF_READING)) {
+		ret = 0;
+		dev_dbg(&dev->pdev->dev, "mei_hbm_dispatch.\n");
+		goto reset_slots;
+	}
 
-		dev_dbg(&dev->pdev->dev, "call mei_amthif_irq_read_msg.\n");
-		dev_dbg(&dev->pdev->dev, MEI_HDR_FMT, MEI_HDR_PRM(mei_hdr));
+	/* find recepient cl */
+	list_for_each_entry(cl, &dev->file_list, link) {
+		if (mei_cl_hbm_equal(cl, mei_hdr)) {
+			cl_dbg(dev, cl, "got a message\n");
+			break;
+		}
+	}
+
+	/* if no recepient cl was found we assume corrupted header\n */
+	if (&cl->link == &dev->file_list) {
+		dev_err(&dev->pdev->dev, "no destination client found 0x%08X\n",
+				dev->rd_msg_hdr);
+		ret = -EBADMSG;
+		goto end;
+	}
+
+	if (mei_hdr->host_addr == dev->iamthif_cl.host_client_id &&
+	    MEI_FILE_CONNECTED == dev->iamthif_cl.state &&
+	    dev->iamthif_state == MEI_IAMTHIF_READING) {
 
 		ret = mei_amthif_irq_read_msg(dev, mei_hdr, cmpl_list);
-		if (ret)
+		if (ret) {
+			dev_err(&dev->pdev->dev, "mei_amthif_irq_read_msg failed = %d\n",
+					ret);
 			goto end;
+		}
 	} else {
-		dev_dbg(&dev->pdev->dev, "call mei_cl_irq_read_msg.\n");
-		dev_dbg(&dev->pdev->dev, MEI_HDR_FMT, MEI_HDR_PRM(mei_hdr));
 		ret = mei_cl_irq_read_msg(dev, mei_hdr, cmpl_list);
-		if (ret)
+		if (ret) {
+			dev_err(&dev->pdev->dev, "mei_cl_irq_read_msg failed = %d\n",
+					ret);
 			goto end;
+		}
 	}
 
+reset_slots:
 	/* reset the number of slots and header */
 	*slots = mei_count_full_read_slots(dev);
 	dev->rd_msg_hdr = 0;
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index 6f577bf..4a7ee81 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -456,6 +456,16 @@ static inline u32 mei_data2slots(size_t length)
 	return DIV_ROUND_UP(sizeof(struct mei_msg_hdr) + length, 4);
 }
 
+/**
+ * mei_slots2data- get data in slots - bytes from slots
+ * @slots -  number of available slots
+ * returns  - number of bytes in slots
+ */
+static inline u32 mei_slots2data(int slots)
+{
+	return slots * 4;
+}
+
 /*
  * mei init function prototypes
  */
-- 
1.8.3.1

RE: [char-misc-next 0/3] mei: driver cleanups

[Winkler, Tomas]

> 
> *** BLURB HERE ***
Oops, sorry for that.
Tomas

Re: [char-misc-next 0/3] mei: driver cleanups

[gregkh]

On Mon, Nov 11, 2013 at 11:35:40AM +0000, Winkler, Tomas wrote:
> 
> 
> > 
> > *** BLURB HERE ***
> Oops, sorry for that.

So, any hint for what these are for?  I can't take patches until
3.13-rc1 is out, which will be a few weeks.  But then, which should this
be for, 3.13 as these are regression fixes, or 3.14 as they are only
"cleanups"?

thanks,

greg k-h

RE: [char-misc-next 0/3] mei: driver cleanups

[Winkler, Tomas]

> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, November 11, 2013 17:02
> To: Winkler, Tomas
> Cc: arnd@arndb.de; linux-kernel@vger.kernel.org
> Subject: Re: [char-misc-next 0/3] mei: driver cleanups
> 
> On Mon, Nov 11, 2013 at 11:35:40AM +0000, Winkler, Tomas wrote:
> >
> >
> > >
> > > *** BLURB HERE ***
> > Oops, sorry for that.
> 
> So, any hint for what these are for?  I can't take patches until
> 3.13-rc1 is out, which will be a few weeks.  But then, which should this
> be for, 3.13 as these are regression fixes, or 3.14 as they are only
> "cleanups"?

No regressions here, I don't mind it will be in 3.14
I will mark fixes 3.13 properly (hopefully w/o BLURB)

Tomas