imx/sdma.c: silence "may be uninitialized" warnings with gcc10.2 -O1

[marc-hb]

gcc10.2 does not see that config->elem_array.count is greater than zero
and complains. This happens only at the -O1 optimization level that is
rarely tested.

error: 'bd' may be used uninitialized in this function
                                  [-Werror=maybe-uninitialized]
  758 |  bd->config &= ~SDMA_BD_CONT;

error: 'width' may be used uninitialized in this function
                                  [-Werror=maybe-uninitialized]
  786 |  watermark = (config->burst_elems * width) / 8;
      |              ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

This is preferred over initialization at the top of the function
that hides all warnings including valid ones.

Of course the even better fix would to BOTH declare and initialization
just before the loop, as late as possible just like in any other modern
and safer programming language. However C99 declarations are not allowed
yet; I've added this to the next TSC agenda:
github.com/orgs/thesofproject/teams/steering-committee/discussions/24

 -693,11 +698,9 @@ static int sdma_prep_desc(struct dma_chan_data *channel,
                          struct dma_sg_config *config)
 {
        int i;
-       int width;
        int watermark;
        uint32_t sdma_script_addr;
        struct sdma_chan *pdata = dma_chan_get_data(channel);
-       struct sdma_bd *bd;

        /* Validate requested configuration */
        if (config->elem_array.count > SDMA_MAX_BDS) {
 -713,6 +716,9 @@ static int sdma_prep_desc(struct dma_chan_data *channel,

        pdata->next_bd = 0;

+       struct sdma_bd *bd = &pdata->desc[0]; // or NULL
+       int width = 0;
+
        for (i = 0; i < config->elem_array.count; i++) {
                bd = &pdata->desc[i];
                /* For MEM2DEV and DEV2MEM, buf_addr holds the RAM address

Signed-off-by: Marc Herbert marc.herbert@intel.com

[marc-hb]

With gcc 10.2, this change generates the exact same binary at the -O2 level.

It makes the .text section 4 bytes bigger at the -O1 level (after removing -Werr) and 8 bytes bigger at the -O0 level.

This got in my way while testing the STATIC_ASSERT variant in PR #4441

[iuliana-prodan]

This looks ok.
But, can we move the initialization up, where the variables are defined?

[marc-hb]

I don't think this is a good idea but please submit an alternative fix and I'll abandon this one.

I don't think this is a good idea for at least two reasons:

1. I placed these fake initializations immediately before the real one below.
2. I placed these fake initializations after the config->elem_array.count <= 0 assert that guarantees that pdata->desc[0] exists (which is surprisingly enough for other optimization levels)

As a consequence:

1. makes sure that the compiler turns this into a no-op and optimizes it away, producing the exact same binary as before (except for debug symbols; use diffoscope or -g0 to compare). Having the real and fake initializations close to each other could also make any refactoring easier.
2. makes sure that, if someone ever tries to use pdata->desc[0] before the config->elem_array.count <= 0 assert in the future by mistake, then the compiler can notice and warn about it.

[dbaluta]

My only objection here is to place the initializations on separate lines.

[iuliana-prodan]

even if we put:

int width = 0;
struct sdma_bd *bd = NULL;

we get the "may be uninitialized" warning?
I believe this is the simplest solution.

[marc-hb]

It's shorter but it's also less safe because it hides all uninitialized warnings, including valid uninitialized warnings.
So let's pretend someone starts to actually use an uninitialized bd after some future refactoring:

@@ -706,15 +706,18 @@ static int sdma_prep_desc(struct dma_chan_data *channel,
 		return -EINVAL;
 	}
 	if (config->elem_array.count <= 0) {
-		tr_err(&sdma_tr, "sdma_set_config: Invalid descriptor count: %d",
-		       config->elem_array.count);
+		tr_err(&sdma_tr, "sdma_set_config: Invalid descriptor count: %d, %p",
+		       config->elem_array.count, bd);
 		return -EINVAL;
 	}

Of course no one would do such a basic mistake, this is just pretending to be some more significant (and more error-prone) refactoring.

With this PR such a mistake is correctly detected by gcc10. With bd = NULL at the top then ALL uninitialized uses are silenced, even valid ones like this one.

Of course the even better fix would to BOTH declare and initialization just before the loop, as late as possible just like in any other modern and safer programming language, see updated commit message.

[dbaluta]

@iuliana-prodan we ok with this?

[iuliana-prodan]

@iuliana-prodan we ok with this?

yes