netdev - Re: [PATCHv5 1/4] modem_shm: Add Modem Access Framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121015212943.GA12540@kroah.com>
Date:	Mon, 15 Oct 2012 14:29:43 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Arun Murthy <arun.murthy@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, alan@...rguk.ukuu.org.uk
Subject: Re: [PATCHv5 1/4] modem_shm: Add Modem Access Framework

On Mon, Oct 15, 2012 at 10:58:37AM +0530, Arun Murthy wrote:

I'm going to ignore your .c logic, as there are things in it that I
don't think is correct.  But it all comes down to your data structures,
if you fix them, then the .c logic will become correct:

> --- /dev/null
> +++ b/include/linux/modem_shm/modem.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * License Terms: GNU General Public License v2
> + * Author: Kumar Sanghvi
> + *	Arun Murthy <arun.murthy@...ricsson.com>
> + *
> + * Heavily adapted from Regulator framework
> + */
> +#ifndef __MODEM_H__
> +#define __MODEM_H__

__MODEM_SHM_MODEM_H__, right?

> +
> +#include <linux/device.h>
> +
> +struct clients {
> +	struct device *dev;

Why is this a pointer?  It should be embedded in the structure.

> +	const char *name;

Why is this needed?  It should be the same as the device, right?

> +	atomic_t cnt;

Why is this needed at all?  And if it's needed, why is it an atomic?
(hint, your use of atomic_t really isn't correct at all in this patch,
it's not doing what you think it is doing...)

> +};

Also, the name of the structure here is _VERY_ generic, that's not
acceptable in the global kernel namespace.  Hint, it probably isn't even
needed to be defined in this .h file at all, right?

> +
> +struct modem_desc {
> +	int (*request)(struct modem_desc *);
> +	void (*release)(struct modem_desc *);
> +	int (*is_requested)(struct modem_desc *);
> +	struct clients *mclients;

Why do you have a pointer to a device, and yet:

> +	struct device *dev;

have a device here?

> +	char *name;

Same *dev and name comment as above.

> +	u8 no_clients;
> +	atomic_t use_cnt;
> +	atomic_t cli_cnt;

Same question about these atomic_t variables, why are they here, and
most importantly, why are they an atomic variable?

thanks,

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