Re: [PATCHv4 1/4] modem_shm: Add Modem Access Framework

On Wed, Oct 03, 2012 at 05:54:08AM +0200, Arun MURTHY wrote:

quoted

On Mon, Oct 01, 2012 at 07:30:38AM +0200, Arun MURTHY wrote:

quoted

quoted

On Fri, Sep 28, 2012 at 01:35:01PM +0530, Arun Murthy wrote:

quoted

+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/printk.h>
+#include <linux/modem_shm/modem.h>
+
+static struct class *modem_class;

What's wrong with a bus_type instead?

Can I know the advantage of using bus_type over class?

You have devices living on a bus, and it's much more descriptive than a class
(which we are going to eventually get rid of one of these days...).

Might I ask why you choose a class over a bus_type?

Basically my requirement is to create a central entity for accessing and releasing
modem from APE.

What is an "APE"?

And what do you mean by "accessing" and "releasing"?

Since this is done by different clients the central entity should
be able to handle the request and play safely, since this has more affect in
system suspend and deep sleep. Using class helps me in achieving this
and also create an entry to user space which can be used in the later
parts. Moreover this not something like a bus or so, so I didn't use
bus instead went with a simple class approach.

But as you have devices that are "binding" to this "controller", a bus
might make more sense, right?

I don't see how a class helps out for you here more than anything else,
what are you expecting from the class interface?  You aren't using the
reference counting logic it provides, so why use it at all?

Actually, why use the driver core at all in the first place if you
aren't needing the devices to show up in sysfs (as you don't have a
device, you are just a mediator)?

quoted

quoted

quoted

quoted

+int modem_release(struct modem_desc *mdesc) {
+	if (!mdesc->release)
+		return -EFAULT;
+
+	if (modem_is_requested(mdesc)) {
+		atomic_dec(&mdesc->mclients->cnt);
+		if (atomic_read(&mdesc->use_cnt) == 1) {
+			mdesc->release(mdesc);
+			atomic_dec(&mdesc->use_cnt);
+		}

Eeek, why aren't you using the built-in reference counting that the
struct device provided to you, and instead are rolling your own?
This happens in many places, why?

My usage of counters over here is for each modem there are many clients.
Each of the clients will have a ref to modem_desc. Each of them use
this for requesting and releasing the modem. One counter for tracking
the request and release for each client which is done by variable 'cnt' in

struct clients.

quoted

The counter use_cnt is used for tracking the modem request/release
irrespective of the clients and counter cli_cnt is used for
restricting the modem_get to the no of clients defined in no_clients.

So totally 3 counter one for restricting the usage of modem_get by
clients, second for restricting modem request/release at top level,
and 3rd for restricting modem release/request for per client per modem

basis.

quoted

Can you let me know if the same can be achieved by using built-in ref
counting?

Yes, because you don't need all of those different levels, just stick with one
and you should be fine. :)

No, checks at all these levels are required, I have briefed out the need also.

I still don't understand, sorry.

This will have effect on system power management, i.e suspend and deep
sleep.

How does power management matter?  If you tie into the driver model
properly, power management comes "for free" so you don't have to do
anything special about it.  Why not use that logic instead of trying to
roll your own?

We restrict that the drivers should request modem only once and release
only once, but we cannot rely on the clients hence a check for the same has
to be done in the MAF.

You can't rely on the clients to do what?  And why can't you rely on
them?  What is going to happen?  Who is a "client" here?  Other kernel
code?

I really don't understand your model at all as to what you are trying to
mediate and manage here, sorry.  I suggest writing it all up as your
first patch (documentation is good), so that we can properly review your
implementation and not argue about how to implement something that I
honestly don't understand.

Also the no of clients should be defined and hence a
check for the same is done in MAF.

Defined where?  What is "MAF"?

Apart from all these the requests coming from all the clients is to be
accumulated and based on that modem release or access should be
performed, hence so.

That sentance makes no sense to me, it must be too early for me here...

greg k-h