iio: adc: ad_sigma_delta: fix CS assertion and registerless device handling#3348
Open
RaduSabau1 wants to merge 2 commits into
Open
iio: adc: ad_sigma_delta: fix CS assertion and registerless device handling#3348RaduSabau1 wants to merge 2 commits into
RaduSabau1 wants to merge 2 commits into
Conversation
In ad_sigma_delta_single_conversion(), set_mode(AD_SD_MODE_IDLE) and disable_one() were called from the out: block while keep_cs_asserted was still true. This caused any SPI transfer issued by those callbacks to carry cs_change=1, leaving CS permanently asserted after the conversion. Fix by moving both calls into the out_unlock: block, after keep_cs_asserted is cleared, matching the pattern already used in ad_sd_calibrate(). In the error path of ad_sd_buffer_postenable(), if an operation fails after set_mode(AD_SD_MODE_CONTINUOUS) has already succeeded (e.g. spi_offload_trigger_enable()), the device is left in continuous conversion mode with CS physically asserted. Additionally, bus_locked remaining true after spi_bus_unlock() causes subsequent SPI operations to call spi_sync_locked() without the bus lock actually held, allowing concurrent SPI access. Fix the error path by clearing keep_cs_asserted first, then calling set_mode(AD_SD_MODE_IDLE) to revert the device mode and deassert CS, then clearing bus_locked before releasing the bus. For devices that implement neither set_mode nor disable_one (such as MAX11205, which has no physical CS pin), no SPI transfer is issued during cleanup and the cs_change flag has no effect on any physical line. Fixes: a64317b ("iio: adc: ad_sigma_delta: Check for previous ready signals") Signed-off-by: Radu Sabau <radu.sabau@analog.com>
…vices ad_sigma_delta_clear_pending_event() falls through to the status register read path for devices with has_registers = false and no rdy_gpiod. For such devices, ad_sd_read_reg() skips the address byte entirely and clocks raw MISO bytes with no address phase — making it byte-for-byte identical to reading conversion data. If a pending conversion result is present, this partially consumes it and corrupts the data stream for the subsequent ad_sd_read_reg() call in ad_sigma_delta_single_conversion(). Furthermore, with num_resetclks = 0 on these devices, data_read_len evaluates to 0. If the clocked byte has bit 7 clear, pending_event is set and the code attempts memset(data + 2, 0xff, 0 - 1), overflowing to SIZE_MAX and corrupting the heap. Fix by returning 0 immediately when neither rdy_gpiod nor has_registers is set. This is safe for all current registerless devices: ad7191 and ad7780 (with powerdown GPIO) are reset between conversions by CS deassertion, so there is no stale result to drain; ad7780 (without powerdown GPIO) and max11205 are continuously-converting and cycle ~DRDY at the output data rate regardless of whether the previous result was read, so the next falling edge fires naturally. A future registerless device that holds ~DRDY asserted until data is read would be broken by this early return and would require either num_resetclks set or a rdy-gpio. The same heap corruption is reachable on any device with rdy_gpiod set but num_resetclks = 0: if the GPIO indicates a pending event, the drain path executes memset(data + 2, 0xff, 0 - 1) regardless of has_registers. Add an explicit data_read_len == 0 guard after the pending event check; the stale result is then consumed by the first ad_sd_read_reg() call in ad_sigma_delta_single_conversion(). Fixes: a64317b ("iio: adc: ad_sigma_delta: Check for previous ready signals") Signed-off-by: Radu Sabau <radu.sabau@analog.com>
RaduSabau1
requested review from
CiprianRegus,
amiclaus,
btogorean,
buha,
chegbeli,
danmois,
dbogdan,
machschmitt,
mhennerich,
nunojsa,
rbolboac and
stefpopa
as code owners
Collaborator
Author
|
is the error related? |
Collaborator
Does not look like it is. Are these coming from upstream? At least the first patch i think it is! Assuming you're doing this given it was accepted, I would prefer to see the cherry-picks from the appropriate tree so that one can see the tags and easily see it is a backport. |
Collaborator
|
it does infer the correct symbol: But it cannot be compile tested in isolation: the symbol is not selectable by itself, but result from any(AD7124, AD7173, ...) being selected. changing the symbol from to that is, if |
Collaborator
|
@RaduSabau1 if you agree with my comment above, please submit upstream |
Collaborator
Author
It would need a separate patch then, since these patches were already applied to the fixes-togreg branch. |
Collaborator
No need to mix things! We can merge this one and have the other change in a different PR. Just make sure to do what I asked wrt to cherry-picking from upstream tree. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Description
Commit 1 fixes CS being left permanently asserted after single conversion
and in the error path of ad_sd_buffer_postenable(). In
ad_sigma_delta_single_conversion(), set_mode(AD_SD_MODE_IDLE) and
disable_one() were executing while keep_cs_asserted was still true,
causing any SPI transfer they issued to carry cs_change=1. The
postenable() error path also failed to call set_mode(AD_SD_MODE_IDLE),
leaving the device in continuous conversion mode with bus_locked
incorrectly set, opening a window for concurrent SPI access.
Commit 2 fixes ad_sigma_delta_clear_pending_event() for devices with
has_registers = false and no rdy_gpiod (currently AD7191, AD7780, and
MAX11205). These devices fall through to the status register read path,
but since has_registers is false, ad_sd_read_reg() transmits no address
byte and blindly clocks raw MISO bytes — indistinguishable from reading
conversion data, partially consuming any pending result and corrupting the
stream. With num_resetclks = 0 on these devices a further hazard exists:
if pending_event is set, the drain path attempts memset of SIZE_MAX bytes,
corrupting the heap. The fix returns 0 immediately for registerless
devices. This is safe for all current instances: AD7191 and AD7780 (with
powerdown GPIO) are reset between conversions by CS deassertion; AD7780
(without powerdown GPIO) and MAX11205 are continuously-converting and
cycle ~DRDY regardless, so the next falling edge fires naturally. A future
registerless device that holds ~DRDY asserted until data is read would
need num_resetclks set or a rdy-gpio instead. The same heap corruption can
be triggered on any device with rdy_gpiod set but num_resetclks = 0, so
an explicit data_read_len == 0 guard is added independently.
Lore : https://patchwork.kernel.org/project/linux-iio/cover/20260527-ad_sigma_delta-fix-v5-0-446fd2bc7330@analog.com/
PR Type
PR Checklist