Re: [bug report] ALSA: firewire-lib: extend tracepoints event including CYCLE_TIME of 1394 OHCI

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

On Fri, Jan 13, 2023 at 01:04:35PM +0300, Dan Carpenter wrote:
> Hello Takashi Sakamoto,
> 
> The patch fef4e61b0b76: "ALSA: firewire-lib: extend tracepoints event
> including CYCLE_TIME of 1394 OHCI" from Jan 10, 2023, leads to the
> following Smatch static checker warning:
> 
> sound/firewire/amdtp-stream.c:944 generate_tx_packet_descs() error: uninitialized symbol 'curr_cycle_time'.
> sound/firewire/amdtp-stream.c:1099 process_rx_packets()	error: uninitialized symbol 'curr_cycle_time'.

Thanks for the report. Indeed, I've absolutely overlooked it. I'll fix
it enough later with an issue in your previous report for ALSA
firewire-motu driver.

Actually, the value of local variable is referred only when tracepoints
event is probed, thus the uninitialized bug never occurs. Nevertheless,
in a point of coding convention, it's preferable to be initialized.

> sound/firewire/amdtp-stream.c
>     1047 static void process_rx_packets(struct fw_iso_context *context, u32 tstamp, size_t header_length,
>     1048                                void *header, void *private_data)
>     1049 {
>     1050         struct amdtp_stream *s = private_data;
>     1051         const struct amdtp_domain *d = s->domain;
>     1052         const __be32 *ctx_header = header;
>     1053         const unsigned int events_per_period = d->events_per_period;
>     1054         unsigned int event_count = s->ctx_data.rx.event_count;
>     1055         struct pkt_desc *desc = s->packet_descs_cursor;
>     1056         unsigned int pkt_header_length;
>     1057         unsigned int packets;
>     1058         u32 curr_cycle_time;
>                  ^^^^^^^^^^^^^^^^^^^
> 
>     1059         bool need_hw_irq;
>     1060         int i;
>     1061 
>     1062         if (s->packet_index < 0)
>     1063                 return;
>     1064 
>     1065         // Calculate the number of packets in buffer and check XRUN.
>     1066         packets = header_length / sizeof(*ctx_header);
>     1067 
>     1068         generate_rx_packet_descs(s, desc, ctx_header, packets);
>     1069 
>     1070         process_ctx_payloads(s, desc, packets);
>     1071 
>     1072         if (!(s->flags & CIP_NO_HEADER))
>     1073                 pkt_header_length = IT_PKT_HEADER_SIZE_CIP;
>     1074         else
>     1075                 pkt_header_length = 0;
>     1076 
>     1077         if (s == d->irq_target) {
>     1078                 // At NO_PERIOD_WAKEUP mode, the packets for all IT/IR contexts are processed by
>     1079                 // the tasks of user process operating ALSA PCM character device by calling ioctl(2)
>     1080                 // with some requests, instead of scheduled hardware IRQ of an IT context.
>     1081                 struct snd_pcm_substream *pcm = READ_ONCE(s->pcm);
>     1082                 need_hw_irq = !pcm || !pcm->runtime->no_period_wakeup;
>     1083         } else {
>     1084                 need_hw_irq = false;
>     1085         }
>     1086 
>     1087         if (trace_amdtp_packet_enabled())
>     1088                 (void)fw_card_read_cycle_time(fw_parent_device(s->unit)->card, &curr_cycle_time);
> 
> No error checking and no else path.
> 
>     1089 
>     1090         for (i = 0; i < packets; ++i) {
>     1091                 struct {
>     1092                         struct fw_iso_packet params;
>     1093                         __be32 header[CIP_HEADER_QUADLETS];
>     1094                 } template = { {0}, {0} };
>     1095                 bool sched_irq = false;
>     1096 
>     1097                 build_it_pkt_header(s, desc->cycle, &template.params, pkt_header_length,
>     1098                                     desc->data_blocks, desc->data_block_counter,
> --> 1099                                     desc->syt, i, curr_cycle_time);
>                                                            ^^^^^^^^^^^^^^^
> Uninitialized.
> 
>     1100 
>     1101                 if (s == s->domain->irq_target) {
>     1102                         event_count += desc->data_blocks;
>     1103                         if (event_count >= events_per_period) {
>     1104                                 event_count -= events_per_period;
>     1105                                 sched_irq = need_hw_irq;
>     1106                         }
>     1107                 }
>     1108 
>     1109                 if (queue_out_packet(s, &template.params, sched_irq) < 0) {
>     1110                         cancel_stream(s);
>     1111                         return;
>     1112                 }
>     1113 
>     1114                 desc = amdtp_stream_next_packet_desc(s, desc);
>     1115         }
>     1116 
>     1117         s->ctx_data.rx.event_count = event_count;
>     1118         s->packet_descs_cursor = desc;
>     1119 }
> 
> regards,
> dan carpenter


Regards

Takashi Sakamoto