Prev: [PATCH] docbook: use ID as filename
Next: Removing dead APM
From: Arnd Bergmann on 5 Jul 2010 11:10
I took another look and found some more things that should be improved.
Overall, the quality of this driver has improved enourmously, and I'm
optimistic that it will be a lot easier for you in your next device
driver with all the details you have learned about the coding style.

On Monday 05 July 2010, Masayuki Ohtak wrote:

> +struct pch_phub_reg {
> + u32 phub_id_reg;
> + u32 q_pri_val_reg;
> + u32 rc_q_maxsize_reg;
> + u32 bri_q_maxsize_reg;
> + u32 comp_resp_timeout_reg;
> + u32 bus_slave_control_reg;
> + u32 deadlock_avoid_type_reg;
> + u32 intpin_reg_wpermit_reg0;
> + u32 intpin_reg_wpermit_reg1;
> + u32 intpin_reg_wpermit_reg2;
> + u32 intpin_reg_wpermit_reg3;
> + u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
> + u32 clkcfg_reg;
> + void __iomem *pch_phub_base_address;
> + void __iomem *pch_phub_extrom_base_address;
> + int pch_phub_suspended;
> +} pch_phub_reg;

The variable you define here is in the global namespace, which it
should not be in. I'd suggest making it static and splitting the
type defintion from the variable definition to make that more obvious,
like:

struct pch_phub_reg {
...
};

static struct pch_phub_reg pch_phub_reg;

> +static DEFINE_MUTEX(pch_phub_ioctl_mutex);
> +static DEFINE_MUTEX(pch_phub_read_mutex);
> +static DEFINE_MUTEX(pch_phub_write_mutex);

Having three mutexes here means that you have effectively no
serialization between the functions at all. The mutex only
has an effect if you use the same one in all three functions!

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 6 Jul 2010 02:40
On Tuesday 06 July 2010 08:20:52 Masayuki Ohtak wrote:
> Hi Arnd
>
> I have modified for your comments.
> Please confirm below.
>

Yes, looks good. Thanks for the quick reply.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Randy Dunlap on 6 Jul 2010 12:00
On 07/05/10 00:20, Masayuki Ohtak wrote:
> Hi Randy
>
> I have modified for your comments.
> Please confirm below.

Agreed, thanks for the updates.

> Thanks, Ohtake.
>
> ---
>
> Packet hub driver of Topcliff PCH
>
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is
> a special converter device in Topcliff PCH that translate AMBA transactions
> to PCI Express transactions and vice versa. Thus packet hub helps present
> all IO peripherals in Topcliff PCH as PCIE devices to IA system.
> Topcliff PCH has MAC address and Option ROM data.
> These data are in SROM which is connected to PCIE bus.
> Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in
> SROM.
>
> Signed-off-by: Masayuki Ohtake <masa-korg(a)dsn.okisemi.com>
> Acked-by: Arnd Bergmann <arnd(a)arndb.de>
>
> ---
> Documentation/ioctl/ioctl-number.txt | 1 +
> drivers/char/Kconfig | 9 +
> drivers/char/Makefile | 2 +
> drivers/char/pch_phub/Makefile | 3 +
> drivers/char/pch_phub/pch_phub.c | 803 ++++++++++++++++++++++++++++++++++
> drivers/char/pch_phub/pch_phub.h | 48 ++
> 6 files changed, 866 insertions(+), 0 deletions(-)
> create mode 100644 drivers/char/pch_phub/Makefile
> create mode 100755 drivers/char/pch_phub/pch_phub.c
> create mode 100755 drivers/char/pch_phub/pch_phub.h
>
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 35cf64d..b700b17 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -320,4 +320,5 @@ Code Seq#(hex) Include File Comments
> <mailto:thomas(a)winischhofer.net>
> 0xF4 00-1F video/mbxfb.h mbxfb
> <mailto:raph(a)8d.com>
> +0xF7 00-0F drivers/char/pch_phub/pch_phub.h PCH Phub driver
> 0xFD all linux/dm-ioctl.h



--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Yong Wang on 6 Jul 2010 21:40
On Tue, Jul 06, 2010 at 08:30:02AM +0200, Arnd Bergmann wrote:
> On Tuesday 06 July 2010 08:20:52 Masayuki Ohtak wrote:
> > Hi Arnd
> >
> > I have modified for your comments.
> > Please confirm below.
> >
>
> Yes, looks good. Thanks for the quick reply.
>
> Arnd

Hi Andrew,

Do you have any further comments on this patch? If so, please let us
know.

Thanks
-Yong

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 9 Jul 2010 16:10
On Tue, 06 Jul 2010 15:20:52 +0900
Masayuki Ohtak <masa-korg(a)dsn.okisemi.com> wrote:

> Hi Arnd
>
> I have modified for your comments.
> Please confirm below.
>
> Thanks, Ohtake.
>
> ---
> Packet hub driver of Topcliff PCH
>
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is
> a special converter device in Topcliff PCH that translate AMBA transactions
> to PCI Express transactions and vice versa. Thus packet hub helps present
> all IO peripherals in Topcliff PCH as PCIE devices to IA system.
> Topcliff PCH has MAC address and Option ROM data.
> These data are in SROM which is connected to PCIE bus.
> Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in
> SROM.

That didn't describe the most important part of the driver: the
userspace interface. We should add here something along the lines of

The driver creates a character device /dev/pch_phub. That device file
supports the following operations:

read(): <document the read operation - seems to read a serial ROM?>
write():<document the write operation - seems to write a serial ROM?>
ioctl():<document the ioctl operation - seems to read/write a MAC address?>

>
> ...
>
> +static ssize_t pch_phub_write(struct file *file, const char __user *buf,
> + size_t size, loff_t *ppos)
> +{
> + unsigned int data;
> + int ret_value1;
> + int ret_value2;
> + int err;
> + unsigned int addr_offset;
> + loff_t pos = *ppos;
> + int ret;
> +
> + ret = mutex_lock_interruptible(&pch_phub_mutex);
> + if (ret) {
> + err = -ERESTARTSYS;
> + goto return_err_nomutex;
> + }
> +
> + for (addr_offset = 0; addr_offset < size; addr_offset++) {
> + ret_value1 = get_user(data, &buf[addr_offset]);
> + if (ret_value1) {
> + err = -EFAULT;
> + goto return_err;
> + }
> +
> + ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + pos,
> + data);

I suspect this function will do strange things if passed an initial
*ppos which is outside the range of the ROM. It looks like it will write
a single byte into the ROM then will bale out.


> + if (ret_value2) {
> + err = ret_value2;
> + goto return_err;
> + }
> +
> + if (PCH_PHUB_OROM_SIZE < pos + addr_offset) {

Is this off-by-one?

> + *ppos += addr_offset;
> + goto return_ok;
> + }
> +
> + }
> +
> + *ppos += addr_offset;
> +
> +return_ok:
> + mutex_unlock(&pch_phub_mutex);
> + return addr_offset;
> +
> +return_err:
> + mutex_unlock(&pch_phub_mutex);
> +return_err_nomutex:
> + return err;
> +}
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH] docbook: use ID as filename
Next: Removing dead APM