Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix AD9361 digital interface tuning #35

Merged
merged 8 commits into from
Jan 23, 2018
Merged

Fix AD9361 digital interface tuning #35

merged 8 commits into from
Jan 23, 2018

Conversation

lclausen-adi
Copy link

No description provided.

commodo
commodo reviewed Jan 23, 2018
View reviewed changes
drivers/iio/adc/ad9361_conv.c Outdated
@@ -401,21 +401,21 @@ static void ad9361_dig_tune_verbose_print(struct ad9361_rf_phy *phy,
{
int i, j;

printk("SAMPL CLK: %lu tuning: %s\n",
printk(KERN_INFO "SAMPL CLK: %lu tuning: %s\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to use the convenience macros ?
pr_info() or pr_cont() ?

no strong preference from me whether to use them or leave this as is

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, I didn't think about this, thanks.

commodo
commodo reviewed Jan 23, 2018
View reviewed changes
drivers/iio/adc/ad9361_conv.c Outdated
@@ -69,11 +84,13 @@ ssize_t ad9361_dig_interface_timing_analysis(struct ad9361_rf_phy *phy,
ret = 1;
}

field[i][j] = ret;
field[j][i] = ret;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curios: is this ok ? the row & col indexes have been switched

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is OK, the order in which they are passed to ad9361_set_intf_delay() was changed as well. This was done to avoid unnecessary transitions to the ALERT state.

@larsclausen
iio: ad9361: Fix tuning diagnostics output
78974c3 
Newer kernels require that a continued printk statement is prefixed with
KERN_CONT, otherwise a newline will be inserted. Use the pr_info() and
pr_cont() macros to print the diagnostic output to make sure that the
proper message level indicators are used.

Also get rid of the extra newline at the end of the output, we don't really
need this.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Make sure to go to ALERT state when feedback clock changes
8772a9a 
When the feedback clock changes, either in phase or frequency, or any other
discontinuities appear in the feedback clock, the AD9361 must go to the
ALERT state.

Otherwise an incorrect internal state might be entered and data on the
transmit path might get corrupted. This issue is visible during interface
tuning where the TX tuning can fail under certain circumstances.

To avoid this issue make sure that the AD9361 is always in the ALERT state
when changing the feedback clock during interface tuning. This makes sure
that the interface tuning always succeeds if there is a valid delay setting
that results in a working interface.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Reduce number of ALERT<->FDD transitions during interfac…
934a4d9 
…e tuning

During digital interface tuning the relative delay between the clock and
data is sweep from the minimum to the maximum value and for each value it
is checked whether the interface works reliably or not.

Lets define the relative delay as clock delay - data delay.

Sweeping is done in two phases:
  1) Clock delay is set to zero and data delay is incremented from 0 to 15.
     This produces a relative delay from 0 to -15.
  2) Data delay is set to zero and clock delay is incremented from 0 to 15.
     This produces a relative delay of 0 to 15.

Changing the clock delay requires a transition to the ALERT state and back
to FDD state. This means during the second phase a lot of transitions are
required.

The same set of relative delay values can be produced by changing the
second phase so that clock delay is set to 15 and then data delay is
decremented from 15 to 0. This reduces the ALERT state transitions that are
necessary to 1.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Add a helper function to get the number of PHY channels
c34fb4c 
We have the same pattern to get the number of PHY channels in a few places.
Add a helper function to replace the duplicated logic.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Consolidate PN checker status checks
5f156e9 
There are multiple places that have the same code to check the PN checker
status. Consolidate those into a common helper function.

This reduces code size and gives consistent behavior.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Rework digital tune function for readability and correct…
1f308b9 
…ness

The ad9361_dig_tune() function is pretty difficult to follow at the moment,
it has many levels of nested conditional statements. The function also has
multiple exit paths which are mostly the same, but with minor differences
and bugs. E.g. not all exits paths properly restore the previous state of
the BIST setup.

Rework the ad9361_dig_tune() by breaking it into 3 sub-functions.

  1) ad9361_dig_tune_delay() that does the actual interface tuning by
     sweeping over all possible delay values.
  2) ad9361_dig_tune_rx() that does the preparatory and cleanup work for RX
     tuning.
  3) ad9361_dig_tune_tx() that does the preparatory and cleanup work for TX
     tuning.

ad9361_dig_tune() itself it left with general preparatory and cleanup work
for interface tuning and calling ad9361_dig_tune_rx() and ad9361_dig_tune_tx().

The function also only has a single exit path now to ensure that it
correctly cleans up in all cases.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
iio: ad9361: Highlight selected delay in interface tuning diagnostics
0a372f6 
For diagnostic purposes it is useful to highlight the selected clock or
data delay. Indicate the chosen delay in the diagnostic by a 'O' character.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@larsclausen
ad9361: ad9361_dig_interface_timing_analysis(): Properly setup and cl…
8277d20 
…eanup

Make sure that all timing analysis relevant configuration parameters have
the right value. E.g. currently bist timing analysis does not work in TDD
mode or when loopback mode is enabled.

Also make sure to restore all configuration parameters to their previous
value after the timing analysis has completed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
@commodo
Copy link
Contributor

commodo commented Jan 23, 2018

commits iio: ad9361: Rework digital tune function for readability and correc & io: ad9361: Make sure to go to ALERT state when feedback clock changes are bit hard to follow ;

but, other than that, LGTM :)

LGTM

@lclausen-adi
Copy link
Author

Yeah, but that's because the existing code is quite messy. Future patches should be easier to follow with things being a bit cleaner now.

@lclausen-adi lclausen-adi merged commit 8277d20 into master Jan 23, 2018
@lclausen-adi lclausen-adi deleted the ad9361_fix_tune branch January 23, 2018 13:27
commodo pushed a commit that referenced this pull request Sep 10, 2020
@ChaitanayaKulkarni @axboe
nvmet: fix oops in pt cmd execution
a2138fd 
In the existing NVMeOF Passthru core command handling on failure of
nvme_alloc_request() it errors out with rq value set to NULL. In the
error handling path it calls blk_put_request() without checking if
rq is set to NULL or not which produces following Oops:-

[ 1457.346861] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 1457.347838] #PF: supervisor read access in kernel mode
[ 1457.348464] #PF: error_code(0x0000) - not-present page
[ 1457.349085] PGD 0 P4D 0
[ 1457.349402] Oops: 0000 [#1] SMP NOPTI
[ 1457.349851] CPU: 18 PID: 10782 Comm: kworker/18:2 Tainted: G           OE     5.8.0-rc4nvme-5.9+ #35
[ 1457.350951] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba5276e3214
[ 1457.352347] Workqueue: events nvme_loop_execute_work [nvme_loop]
[ 1457.353062] RIP: 0010:blk_mq_free_request+0xe/0x110
[ 1457.353651] Code: 3f ff ff ff 83 f8 01 75 0d 4c 89 e7 e8 1b db ff ff e9 2d ff ff ff 0f 0b eb ef 66 8
[ 1457.355975] RSP: 0018:ffffc900035b7de0 EFLAGS: 00010282
[ 1457.356636] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000002
[ 1457.357526] RDX: ffffffffa060bd05 RSI: 0000000000000000 RDI: 0000000000000000
[ 1457.358416] RBP: 0000000000000037 R08: 0000000000000000 R09: 0000000000000000
[ 1457.359317] R10: 0000000000000000 R11: 000000000000006d R12: 0000000000000000
[ 1457.360424] R13: ffff8887ffa68600 R14: 0000000000000000 R15: ffff8888150564c8
[ 1457.361322] FS:  0000000000000000(0000) GS:ffff888814600000(0000) knlGS:0000000000000000
[ 1457.362337] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1457.363058] CR2: 0000000000000000 CR3: 000000081c0ac000 CR4: 00000000003406e0
[ 1457.363973] Call Trace:
[ 1457.364296]  nvmet_passthru_execute_cmd+0x150/0x2c0 [nvmet]
[ 1457.364990]  process_one_work+0x24e/0x5a0
[ 1457.365493]  ? __schedule+0x353/0x840
[ 1457.365957]  worker_thread+0x3c/0x380
[ 1457.366426]  ? process_one_work+0x5a0/0x5a0
[ 1457.366948]  kthread+0x135/0x150
[ 1457.367362]  ? kthread_create_on_node+0x60/0x60
[ 1457.367934]  ret_from_fork+0x22/0x30
[ 1457.368388] Modules linked in: nvme_loop(OE) nvmet(OE) nvme_fabrics(OE) null_blk nvme(OE) nvme_corer
[ 1457.368414]  ata_piix crc32c_intel virtio_pci libata virtio_ring serio_raw t10_pi virtio floppy dm_]
[ 1457.380849] CR2: 0000000000000000
[ 1457.381288] ---[ end trace c6cab61bfd1f68fd ]---
[ 1457.381861] RIP: 0010:blk_mq_free_request+0xe/0x110
[ 1457.382469] Code: 3f ff ff ff 83 f8 01 75 0d 4c 89 e7 e8 1b db ff ff e9 2d ff ff ff 0f 0b eb ef 66 8
[ 1457.384749] RSP: 0018:ffffc900035b7de0 EFLAGS: 00010282
[ 1457.385393] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000002
[ 1457.386264] RDX: ffffffffa060bd05 RSI: 0000000000000000 RDI: 0000000000000000
[ 1457.387142] RBP: 0000000000000037 R08: 0000000000000000 R09: 0000000000000000
[ 1457.388029] R10: 0000000000000000 R11: 000000000000006d R12: 0000000000000000
[ 1457.388914] R13: ffff8887ffa68600 R14: 0000000000000000 R15: ffff8888150564c8
[ 1457.389798] FS:  0000000000000000(0000) GS:ffff888814600000(0000) knlGS:0000000000000000
[ 1457.390796] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1457.391508] CR2: 0000000000000000 CR3: 000000081c0ac000 CR4: 00000000003406e0
[ 1457.392525] Kernel panic - not syncing: Fatal exception
[ 1457.394138] Kernel Offset: disabled
[ 1457.394677] ---[ end Kernel panic - not syncing: Fatal exception ]---

We fix this Oops by adding a new goto label out_put_req and reordering
the blk_put_request call to avoid calling blk_put_request() with rq
value is set to NULL. Here we also update the rest of the code
accordingly.

Fixes: 06b7164dfdc0 ("nvmet: add passthru code to process commands")
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@commodo commodo commodo left review comments

@mhennerich mhennerich mhennerich approved these changes

@dbogdan dbogdan dbogdan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lclausen-adi @commodo @mhennerich @dbogdan @larsclausen