| 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
| ||
|
Date: Tue, 03 Jul 2018 21:29:52 +0530
From: Balakrishna Godavarthi <bgodavar@...eaurora.org>
To: Matthias Kaehlcke <mka@...omium.org>
Cc: marcel@...tmann.org, johan.hedberg@...il.com,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-bluetooth@...r.kernel.org, rtatiya@...eaurora.org,
hemantg@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for
setting UART speed
Hi Matthias,
On 2018-06-30 02:31, Matthias Kaehlcke wrote:
> On Fri, Jun 29, 2018 at 08:59:38PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>>
>> On 2018-06-27 00:32, Matthias Kaehlcke wrote:
>> > On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-06-26 05:13, Matthias Kaehlcke wrote:
>> > > > This is a nice improvement, a few remaining questions inline.
>> > > >
>> > > > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
>> > > > > In function qca_setup, we set initial and operating speeds for
>> > > > > Qualcomm
>> > > > > Bluetooth SoC's. This block of code is common across different
>> > > > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> > > > > a wrapper function to set the speeds. So that future coming SoC's
>> > > > > can use these wrapper functions to set speeds.
>> > > > >
>> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@...eaurora.org>
>> > > > > ---
>> > > > > Changes in v8:
>> > > > > * common function to set INIT and operating speeds.
>> > > > > * moved hardware flow control to qca_set_speed().
>> > > > >
>> > > > > Changes in v7:
>> > > > > * initial patch
>> > > > > * created wrapper functions for init and operating speeds.
>> > > > > ---
>> > > > > drivers/bluetooth/hci_qca.c | 89
>> > > > > +++++++++++++++++++++++++++----------
>> > > > > 1 file changed, 65 insertions(+), 24 deletions(-)
>> > > > >
>> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > > index fe62420ef838..38b7dbe6c897 100644
>> > > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > > @@ -119,6 +119,11 @@ struct qca_data {
>> > > > > u64 votes_off;
>> > > > > };
>> > > > >
>> > > > > +enum qca_speed_type {
>> > > > > + QCA_INIT_SPEED = 1,
>> > > > > + QCA_OPER_SPEED
>> > > > > +};
>> > > > > +
>> > > > > struct qca_serdev {
>> > > > > struct hci_uart serdev_hu;
>> > > > > struct gpio_desc *bt_en;
>> > > > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
>> > > > > hci_uart *hu, unsigned int speed)
>> > > > > hci_uart_set_baudrate(hu, speed);
>> > > > > }
>> > > > >
>> > > > > +static unsigned int qca_get_speed(struct hci_uart *hu,
>> > > > > + enum qca_speed_type speed_type)
>> > > > > +{
>> > > > > + unsigned int speed = 0;
>> > > > > +
>> > > > > + if (speed_type == QCA_INIT_SPEED) {
>> > > > > + if (hu->init_speed)
>> > > > > + speed = hu->init_speed;
>> > > > > + else if (hu->proto->init_speed)
>> > > > > + speed = hu->proto->init_speed;
>> > > > > + } else {
>> > > > > + if (hu->oper_speed)
>> > > > > + speed = hu->oper_speed;
>> > > > > + else if (hu->proto->oper_speed)
>> > > > > + speed = hu->proto->oper_speed;
>> > > > > + }
>> > > > > +
>> > > > > + return speed;
>> > > > > +}
>> > > > > +
>> > > > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> > > > > speed_type)
>> > > > > +{
>> > > > > + unsigned int speed, qca_baudrate;
>> > > > > + int ret;
>> > > > > +
>> > > > > + if (speed_type == QCA_INIT_SPEED) {
>> > > > > + speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> > > > > + if (speed)
>> > > > > + host_set_baudrate(hu, speed);
>> > > > > + else
>> > > > > + bt_dev_err(hu->hdev, "Init speed should be non zero");
>> > > >
>> > > > The check for 'speed == 0' is done in multiple places. From this
>> > > > code I deduce that it is expected that both INIT and OPER speed are
>> > > > set to non-zero values. What happens if either of them is zero? Is the
>> > > > driver still operational?
>> > > >
>> > > [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
>> > > qca_setup().
>> > > we actually set baudrate, but int above code missed to
>> > > restrict the
>> > > further call to qca_setup()
>> > > if speed =0. so we are checking the same in the qca_setup()..
>> > > i.e.
>> > > qca_get_speed().
>> >
>> > Sorry, didn't quite get you here. Yes, the driver is still operational?
>> >
>> > Breaking it down in multiple questions:
>> >
>> > 1. Is it an error if one of the speeds isn't specified?
>> >
>>
>> [Bala]: i want to break this issue for two chips.
>> for rome: No, it is not a error if one of speeds are missing.
>> but it is mandate to have at least one of the
>> speeds.
>> to current implementation we are strictly not
>> checking
>> this case.
>> i.e. terminate qca_setup() if both speeds missing.
>> will do this change in this patch.
>> for wnc3990: yes it an error if any of the speeds are missing.
>> it is mandate to have both the speeds.
>> to current implementation we are strictly not
>> checking
>> this case.
>> i.e. terminate qca_setup() if any speeds missing.
>> will integrate this in patch add support for
>> wcn3990.
>>
>> "The check for 'speed == 0' is done in multiple places. From
>> this
>> code I deduce that it is expected that both INIT and OPER speed
>> are
>> set to non-zero values. What happens if either of them is zero?
>> Is
>> the
>> driver still operational"
>>
>> still we will have speed ==0 check for BT chip ROME. pls find
>> code
>> snippet below
>>
>> thanks for catching this corner case.
>
> Thanks for the clarification
>
>> > If yes we should probably check this early once and return an error
>> > early, instead of doing the check repeatedly
>> >
>> > 2. If it is not an error, what is the driver supposed to do?
>> >
>> > > > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
>> > > > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
>> > > > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
>> > > > interesting but finally it isn't done in this series. Did you
>> > > > encounter that it is not feasible/desirable for some reason?
>> > > >
>> > >
>> > > [Bala]: this patch is for rome where flow control is not used.
>> > > after we integrate wcn3990, flow control is hidden in the
>> > > qca_set_speed()
>> > > Pls check [v8 7/7] patch.
>> >
>> > Sorry, my confusion
>> >
>> > > > > static int qca_setup(struct hci_uart *hu)
>> > > > > {
>> > > > > struct hci_dev *hdev = hu->hdev;
>> > > > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
>> > > > > clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> > > > >
>> > > > > /* Setup initial baudrate */
>> > > > > - speed = 0;
>> > > > > - if (hu->init_speed)
>> > > > > - speed = hu->init_speed;
>> > > > > - else if (hu->proto->init_speed)
>> > > > > - speed = hu->proto->init_speed;
>> > > > > -
>> > > > > - if (speed)
>> > > > > - host_set_baudrate(hu, speed);
>> > > > > + qca_set_speed(hu, QCA_INIT_SPEED);
>> > > > >
>> > > > > /* Setup user speed if needed */
>> > > > > - speed = 0;
>> > > > > - if (hu->oper_speed)
>> > > > > - speed = hu->oper_speed;
>> > > > > - else if (hu->proto->oper_speed)
>> > > > > - speed = hu->proto->oper_speed;
>> > > > > + ret = qca_set_speed(hu, QCA_OPER_SPEED);
>> > > > > + if (ret)
>> > > > > + return ret;
>> > > > >
>> > > > > - if (speed) {
>> > > > > + speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> > > > > + if (speed)
>> > > > > qca_baudrate = qca_get_baudrate_value(speed);
>> > > >
>> > > > Is the check here necessary? qca_get_baudrate_value() returns
>> > > > QCA_BAUDRATE_115200 for a zero speed.
>> > >
>> > > this if for no zero operating speed,
>> >
>> > My point is:
>> >
>> > static int qca_setup(struct hci_uart *hu)
>> > {
>> > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> >
>> > ...
>> >
>> > if (speed)
>> > qca_baudrate = qca_get_baudrate_value(speed);
>> > }
>> >
>> > static uint8_t qca_get_baudrate_value(int speed)
>> > {
>> > switch (speed) {
>> >
>> > ...
>> >
>> > default:
>> > return QCA_BAUDRATE_115200;
>> > }
>> > }
>> >
>> > If qca_get_baudrate_value() is called with 'speed == 0' it returns
>> > QCA_BAUDRATE_115200, which is the same value with which
>> > QCA_BAUDRATE_115200 is initialized.
>> >
>> > It seems the initialization and the check for 'speed == 0' could be
>> > removed.
>>
>> [Bala]: the above speed == 0 is highly recommended, let us assume ROME
>> chip
>> init speed moved from 115200 to 2000000 and operating speed is defined
>> as
>> zero.
>> then this check will help us. else if we remove speed ==0 then
>> in
>> qca_baudrate we will end up having a different value.
>> Pls find the code snippet for better understanding.
>
> Hm, still not clear to me, I'm no Bluetooth expert, maybe I'm missing
> something that seems obvious to others.
>
> What exactly do you mean with "init speed moved from 115200 to
> 2000000"?
>
> Is it that the first qca_set_speed(hu, QCA_INIT_SPEED) call in
> qca_setup() uses hu->proto->init_speed and the second one
> hu->init_speed after it was initialized in qca_open()?
>
> I saw you added a new assignment of qca_baudrate below, which probably
> is related, though you don't mention it here:
>
> /* Setup initial baudrate */
> qca_set_speed(hu, QCA_INIT_SPEED);
> speed = qca_get_speed(hu, QCA_INIT_SPEED);
> if (speed)
> qca_baudrate = qca_get_baudrate_value(speed);
>
>
> I find it really hard to discuss in code snippets, it's probably best
> to send a new version of the patch and discuss it with the full
> context.
>
> Regarding the new qca_get_speed() and assignment of qca_baudrate: if
> this needs to be done it should probably be moved just before this
> block
> to keep things together:
>
> speed = qca_get_speed(hu, QCA_OPER_SPEED);
> if (speed)
> qca_baudrate = qca_get_baudrate_value(speed);
>
> If I didn't lose myself jumping back and forth between the snippets
> and v8 qca_baudrate isn't used before. And if that is correct then it
> probably shouldn't be moved before the block, but rewritten to:
>
> speed = qca_get_speed(hu, QCA_OPER_SPEED);
> if (!speed)
> // Note mka@: no need to check 'speed', we know at
> // least one of them is set
> speed = qca_get_speed(hu, QCA_INIT_SPEED);
>
> qca_baudrate = qca_get_baudrate_value(speed);
>
> Not sure if that's correct, but it seems reasonable in the sense that
> you said that at least one of the speeds needs to be set and it
> certainly looks cleaner than the spread out _get_speed() and
> qca_get_baudrate_value(). But maybe I just got it wrong and reality is
> more ugly, best sent a new patch and have the full context.
>
>> after your suggestion. code snippet along with wcn3990 integration.
>>
>>
>> [Bala]: for rome any one of the speeds are required.
>> for wcn3990 requires both speeds.
>>
>> static int qca_check_speeds(struct hci_uart *hu)
>> {
>> struct qca_serdev *qcadev;
>>
>> qcadev = serdev_device_get_drvdata(hu->serdev);
>> if (qcadev->btsoc_type == QCA_WCN3990) {
>> /* QCA WCN3990 requires both the speed values. */
>> if (qca_get_speed(hu, QCA_INIT_SPEED) &&
>> qca_get_speed(hu, QCA_OPER_SPEED))
>>
>> return 0;
>>
>> bt_dev_err(hu->hdev, "Both the speeds should be non
>> zero");
>> return 1;
>> }
>
> It would probably be clearer to invert the logic, and check for the
> error condition and return 0 in the main branch (or even better, at
> the end of the function).
>
>
>> if (qca_get_speed(hu, QCA_INIT_SPEED) ||
>> qca_get_speed(hu, QCA_OPER_SPEED))
>> return 0;
>>
>> bt_dev_err(hu->hdev, "either of the speeds should be non
>> zero");
>> return 1;
>
> ditto
>
>> [Bala]: pls suggest, what could be error no if above criteria
>> is not
>> met.
>
> I think you could use -EINVAL as it would be a configuration error.
>
>> static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> speed_type)
>> {
>> struct qca_serdev *qcadev;
>> unsigned int speed, qca_baudrate;
>> int ret;
>>
>> if (speed_type == QCA_INIT_SPEED) {
>> speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> if (speed)
>> host_set_baudrate(hu, speed);
>> [Bala]: this speed check is required for ROME, that if
>> ROME
>> init speed is zero.
>> return 0;
>> }
>
> Just noticed, I think this would be clearer with an else branch. The
> current structure might be based on a comment from me on an earlier
> version of the _set/get_speed() rework, where I suggested to return to
> save a level of indentation. Here we set either INIT or OPER speed,
> not having the else branch can give the impression that both might be
> set.
>
>> speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> if (!speed)
>> return 0;
>> [Bala]: this speed check is required for ROME, that if ROME
>> operating speed is zero.
>>
>> [Bala]: this is speeds in both OPER and init speeds are required.. as
>> we
>> move forward if any one of the speed is set.
>>
>> qcadev = serdev_device_get_drvdata(hu->serdev);
>> /* Disabling hardware flow control is preferred while
>> * sending change baud rate command to SoC.
>> if (qcadev->btsoc_type == QCA_WCN3990)
>> hci_uart_set_flow_control(hu, true);
>>
>> qca_baudrate = qca_get_baudrate_value(speed);
>> bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>> ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> if (ret) {
>> bt_dev_err(hu->hdev, "Failed to change the baudrate
>> (%d)",
>> ret);
>> return ret;
>> }
>>
>> host_set_baudrate(hu, speed);
>> if (qcadev->btsoc_type == QCA_WCN3990)
>> hci_uart_set_flow_control(hu, false);
>>
>> return 0;
>> }
>>
>> static int qca_setup(struct hci_uart *hu)
>> {
>> struct hci_dev *hdev = hu->hdev;
>> struct qca_data *qca = hu->priv;
>> unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> struct qca_serdev *qcadev;
>> int ret;
>> int soc_ver = 0;
>>
>> qcadev = serdev_device_get_drvdata(hu->serdev);
>>
>> /* Patch downloading has to be done without IBS mode */
>> clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>>
>> ret = qca_check_speeds(hu);
>> if (ret)
>> return ret;
>>
>> /* Setup initial baudrate */
>> qca_set_speed(hu, QCA_INIT_SPEED);
>> speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> if (speed)
>> qca_baudrate = qca_get_baudrate_value(speed);
>>
>> [Bala]: i have newly add above statement. initial speed of both ROME
>> and
>> wcn3990 when the chip boot up is 115200.
>> in future, if we have new chip where initial speed of chip
>> during
>> boot up is 200000. then this will help us to get the actual initial
>> speed
>> i.e. based upon the init speed.
>
> As mentioned above, this should probably move further down.
>
>> if (qcadev->btsoc_type == QCA_WCN3990) {
>> bt_dev_dbg(hdev, "setting up wcn3990");
>> hci_uart_set_flow_control(hu, true);
>> ret = qca_send_vendor_cmd(hdev,
>> QCA_WCN3990_POWERON_PULSE);
>> if (ret)
>> return ret;
>>
>> hci_uart_set_flow_control(hu, false);
>> serdev_device_close(hu->serdev);
>> ret = serdev_device_open(hu->serdev);
>> if (ret) {
>> bt_dev_err(hdev, "failed to open port");
>> return ret;
>> }
>>
>> msleep(100);
>> /* Setup initial baudrate */
>> qca_set_speed(hu, QCA_INIT_SPEED);
>> hci_uart_set_flow_control(hu, false);
>> ret = qca_read_soc_version(hdev, &soc_ver);
>> if (ret < 0 || soc_ver == 0) {
>> bt_dev_err(hdev, "Failed to get version %d",
>> ret);
>> return ret;
>> }
>> bt_dev_info(hdev, "wcn3990 controller version 0x%08x",
>> soc_ver);
>> } else {
>> bt_dev_info(hdev, "ROME setup");
>> }
>>
>> /* Setup user speed if needed */
>> ret = qca_set_speed(hu, QCA_OPER_SPEED);
>> if (ret)
>> return ret;
>>
>> speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> if (speed)
>> qca_baudrate = qca_get_baudrate_value(speed);
>>
>> [Bala]: the above speed == 0 is highly recommended, let us
>> assume ROME
>> chip init speed moved from 115200 to 2000000 and operating speed is
>> defined
>> as zero.
>> then this check will help us. else if we remove speed ==0 then
>> in
>> qca_baudrate we will end up having a different value.
>> ….
>> }
>>
>> Pls let me know, whether i have clarified your queries.
>
> Partially, let's continue the discussion on v9 unless you have
> questions/comments on my comments before you post.
>
> Thanks
>
> Matthias
Thanks for inputs, will fix your comments and send you v9 patch set
tomorrow.
--
Regards
Balakrishna.
Powered by blists - more mailing lists