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
