linux-kernel - RE: [PATCH 08/11] Bluetooth: Add support for CG2900 UART

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] [day] [month] [year] [list]

Date:	Fri, 17 Dec 2010 13:29:11 +0100
From:	Par-Gunnar HJALMDAHL <par-gunnar.p.hjalmdahl@...ricsson.com>
To:	Vitaly Wool <vitalywool@...il.com>
Cc:	Pavan Savoy <pavan_savoy@...y.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Marcel Holtmann <marcel@...tmann.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	Lukasz Rymanowski <Lukasz.Rymanowski@...to.com>,
	Linus WALLEIJ <linus.walleij@...ricsson.com>,
	Par-Gunnar Hjalmdahl <pghatwork@...il.com>
Subject: RE: [PATCH 08/11] Bluetooth: Add support for CG2900 UART

Hi Vitaly,

> -----Original Message-----
> From: Vitaly Wool [mailto:vitalywool@...il.com]
> Sent: den 17 december 2010 13:03
> To: Par-Gunnar HJALMDAHL
> Cc: Pavan Savoy; Alan Cox; Arnd Bergmann; Samuel Ortiz; Marcel
> Holtmann; linux-kernel@...r.kernel.org; linux-
> bluetooth@...r.kernel.org; Lukasz Rymanowski; Linus WALLEIJ; Par-Gunnar
> Hjalmdahl
> Subject: Re: [PATCH 08/11] Bluetooth: Add support for CG2900 UART
> 
> Hi Par,
> 
> so on the top level: this is yet another H4 implementation plus
> channel-based packet routing, right?
> 
> Could you please also elaborate
> 

Yes, the low-level basis is similar to e.g. hci_h4.c, where we register to N_HCI line discipline and then use the first byte to separate between different channels.
While hci_h4.c supports only the standardized Bluetooth H4 channels, the cg2900_uart targets also the CG2900 specific H4 channels.

> More comments on the code are inlined.
> 
> > +#define MAIN_DEV               (uart_info->dev)
> 
> What is that for?
> 

This is just a simplification when using for example dev_err(). It's just to shorten the text (not much in this case but there are other files where it looks better).
No big problem to fix if you want that.

> > + * cg2900_uart_suspend() - Called by Linux PM to put the device in a
> low power mode.
> > + * @pdev:      Pointer to platform device.
> > + * @state:     New state.
> > + *
> > + * In UART case, CG2900 driver does nothing on suspend.
> > + *
> > + * Returns:
> > + *   0 - Success.
> > + */
> > +static int cg2900_uart_suspend(struct platform_device *pdev,
> pm_message_t state)
> > +{
> > +       struct uart_info *uart_info = dev_get_drvdata(&pdev->dev);
> > +
> > +       if (uart_info->sleep_state == CHIP_POWERED_DOWN)
> > +               return 0;
> > +
> > +       if (uart_info->sleep_state != CHIP_ASLEEP)
> > +               return -EBUSY;
> > +
> > +       dev_dbg(MAIN_DEV, "New sleep_state: CHIP_SUSPENDED\n");
> > +       uart_info->sleep_state = CHIP_SUSPENDED;
> > +       return 0;
> > +}
> 
> I don't think this is safe wrt work queue. What if it gets scheduled
> when drivers are suspended?
> 
> Thanks,
>    Vitaly

I'm not 100% sure what you mean since I don't think sleep_state should ever be in CHIP_ASLEEP if we have a work ongoing or in the queue, but I will ask our low power expert to look into this.

/P-G
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/