Message-Id: <200804072000.55637.vda.linux@googlemail.com>
Date:	Mon, 7 Apr 2008 20:00:55 +0200
From:	Denys Vlasenko <vda.linux@...glemail.com>
To:	Hannes Reinecke <hare@...e.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] debloat aic7xxx and aic79xx drivers

On Monday 07 April 2008 12:34, Hannes Reinecke wrote:
> > Adds statics, #ifdefs out huge amount of unused code, adds consts
> > 
> > Signed-off-by: Denys Vlasenko <vda.linux@...glemail.com>
> > --
> > vda
> > 
> NACK. We need the #defines to print out the registers.
> And in either case the *_shipped files are in fact
> autogenerated by aic assembler, so we need to fix that
> one, too.

I assume you are talking about this part of a patch:

--- linux-2.6.25-rc6-aic1/drivers/scsi/aic7xxx/aic79xx_reg.h_shipped    2008-03-23 00:43:20.000000000 +0100
+++ linux-2.6.25-rc6-aic2/drivers/scsi/aic7xxx/aic79xx_reg.h_shipped    2008-03-23 00:54:59.000000000 +0100
@@ -11,23 +11,18 @@ typedef struct ahd_reg_parse_entry {
        uint8_t  value;
        uint8_t  mask;
 } ahd_reg_parse_entry_t;

+#if 0 /* unused */
+
 #if AIC_DEBUG_REGISTERS
 ahd_reg_print_t ahd_mode_ptr_print;
 #else
 #define ahd_mode_ptr_print(regvalue, cur_col, wrap) \
     ahd_print_register(NULL, 0, "MODE_PTR", 0x00, regvalue, cur_col, wrap)
 #endif

 #if AIC_DEBUG_REGISTERS
-ahd_reg_print_t ahd_intstat_print;
-#else
-#define ahd_intstat_print(regvalue, cur_col, wrap) \
-    ahd_print_register(NULL, 0, "INTSTAT", 0x01, regvalue, cur_col, wrap)
-#endif
-
-#if AIC_DEBUG_REGISTERS
 ahd_reg_print_t ahd_seqintcode_print;
 #else
 #define ahd_seqintcode_print(regvalue, cur_col, wrap) \
     ahd_print_register(NULL, 0, "SEQINTCODE", 0x02, regvalue, cur_col, wrap)
@@ -75,22 +70,8 @@ ahd_reg_print_t ahd_hescb_qoff_print;
     ahd_print_register(NULL, 0, "HESCB_QOFF", 0x08, regvalue, cur_col, wrap)
 #endif

 #if AIC_DEBUG_REGISTERS
-ahd_reg_print_t ahd_hs_mailbox_print;
-#else
-#define ahd_hs_mailbox_print(regvalue, cur_col, wrap) \
-    ahd_print_register(NULL, 0, "HS_MAILBOX", 0x0b, regvalue, cur_col, wrap)
-#endif
-
-#if AIC_DEBUG_REGISTERS
-ahd_reg_print_t ahd_seqintstat_print;
-#else
-#define ahd_seqintstat_print(regvalue, cur_col, wrap) \
-    ahd_print_register(NULL, 0, "SEQINTSTAT", 0x0c, regvalue, cur_col, wrap)
-#endif
-
-#if AIC_DEBUG_REGISTERS
.....
.....


Let me explain what I am doing here. I am NOT deleting ahd_intstat_print
definition, I am moving it below the #endif which terminates big
#if 0 /* unused */  block, moving to this place:


@@ -2377,8 +2043,346 @@ ahd_reg_print_t ahd_scb_disconnected_lis
 #define ahd_scb_disconnected_lists_print(regvalue, cur_col, wrap) \
     ahd_print_register(NULL, 0, "SCB_DISCONNECTED_LISTS", 0x1b8, regvalue, cur_col, wrap)
 #endif

+#endif /* unused */
+
+#if AIC_DEBUG_REGISTERS
+ahd_reg_print_t ahd_intstat_print;
+#else
+#define ahd_intstat_print(regvalue, cur_col, wrap) \
+    ahd_print_register(NULL, 0, "INTSTAT", 0x01, regvalue, cur_col, wrap)
+#endif
...
...


#if 0 / #endif block ends up containing definitions of 290 functions/macros,
none of which is EVER used. I used this script (below) and verified that
they are never mentioned anywhere outside of *_shipped files.
I also did test builds with and without debug enabled and they build
with no problems. No undefined references!

cd $KERNEL_TREE/drivers/scsi/aic7xxx/ || exit 1

echo -n "\
ahd_mode_ptr_print
ahd_seqintcode_print
ahd_clrint_print
ahd_error_print
ahd_clrerr_print
ahd_hcntrl_print
ahd_hnscb_qoff_print
ahd_hescb_qoff_print
ahd_clrseqintstat_print
ahd_swtimer_print
ahd_snscb_qoff_print
ahd_sescb_qoff_print
ahd_sdscb_qoff_print
ahd_qoff_ctlsta_print
ahd_dscommand0_print
ahd_arbctl_print
ahd_sg_cache_pre_print
ahd_lqin_print
ahd_typeptr_print
ahd_tagptr_print
ahd_lunptr_print
ahd_datalenptr_print
ahd_statlenptr_print
ahd_cmdlenptr_print
ahd_attrptr_print
ahd_flagptr_print
ahd_cmdptr_print
ahd_qnextptr_print
ahd_idptr_print
ahd_abrtbyteptr_print
ahd_abrtbitptr_print
ahd_maxcmdbytes_print
ahd_maxcmd2rcv_print
ahd_shortthresh_print
ahd_lunlen_print
ahd_cdblimit_print
ahd_maxcmd_print
ahd_maxcmdcnt_print
ahd_lqrsvd01_print
ahd_lqrsvd16_print
ahd_lqrsvd17_print
ahd_cmdrsvd0_print
ahd_lqctl0_print
ahd_lqctl1_print
ahd_scsbist0_print
ahd_lqctl2_print
ahd_scsbist1_print
ahd_sxfrctl0_print
ahd_dlcount_print
ahd_businitid_print
ahd_sxfrctl1_print
ahd_bustargid_print
ahd_sxfrctl2_print
ahd_scsisigo_print
ahd_multargid_print
ahd_scsidat0_img_print
ahd_scsidat_print
ahd_targidin_print
ahd_optionmode_print
ahd_sblkctl_print
ahd_clrsint0_print
ahd_clrsint1_print
ahd_simode2_print
ahd_clrsint2_print
ahd_lqistate_print
ahd_lqostate_print
ahd_clrlqiint0_print
ahd_lqimode0_print
ahd_lqimode1_print
ahd_clrlqiint1_print
ahd_simode3_print
ahd_clrsint3_print
ahd_clrlqoint0_print
ahd_lqomode0_print
ahd_lqomode1_print
ahd_clrlqoint1_print
ahd_os_space_cnt_print
ahd_gsfifo_print
ahd_lqoscsctl_print
ahd_nextscb_print
ahd_clrseqintsrc_print
ahd_currscb_print
ahd_crccontrol_print
ahd_dfftag_print
ahd_lastscb_print
ahd_scsitest_print
ahd_iopdnctl_print
ahd_shaddr_print
ahd_negoaddr_print
ahd_dgrpcrci_print
ahd_negperiod_print
ahd_packcrci_print
ahd_negoffset_print
ahd_negppropts_print
ahd_negconopts_print
ahd_annexcol_print
ahd_annexdat_print
ahd_scschkn_print
ahd_iownid_print
ahd_pll960ctl0_print
ahd_shcnt_print
ahd_townid_print
ahd_pll960ctl1_print
ahd_pll960cnt0_print
ahd_xsig_print
ahd_pll400ctl0_print
ahd_fairness_print
ahd_pll400ctl1_print
ahd_unfairness_print
ahd_pll400cnt0_print
ahd_haddr_print
ahd_plldelay_print
ahd_hodmaadr_print
ahd_hodmacnt_print
ahd_hcnt_print
ahd_hodmaen_print
ahd_scbhaddr_print
ahd_sghaddr_print
ahd_scbhcnt_print
ahd_sghcnt_print
ahd_dff_thrsh_print
ahd_romaddr_print
ahd_romcntrl_print
ahd_romdata_print
ahd_cmcrxmsg0_print
ahd_roenable_print
ahd_ovlyrxmsg0_print
ahd_dchrxmsg0_print
ahd_ovlyrxmsg1_print
ahd_nsenable_print
ahd_cmcrxmsg1_print
ahd_dchrxmsg1_print
ahd_dchrxmsg2_print
ahd_cmcrxmsg2_print
ahd_ost_print
ahd_ovlyrxmsg2_print
ahd_dchrxmsg3_print
ahd_ovlyrxmsg3_print
ahd_cmcrxmsg3_print
ahd_pcixctl_print
ahd_ovlyseqbcnt_print
ahd_dchseqbcnt_print
ahd_cmcseqbcnt_print
ahd_cmcspltstat0_print
ahd_dchspltstat0_print
ahd_ovlyspltstat0_print
ahd_cmcspltstat1_print
ahd_ovlyspltstat1_print
ahd_dchspltstat1_print
ahd_sgrxmsg0_print
ahd_slvspltoutadr0_print
ahd_sgrxmsg1_print
ahd_slvspltoutadr1_print
ahd_sgrxmsg2_print
ahd_slvspltoutadr2_print
ahd_sgrxmsg3_print
ahd_slvspltoutadr3_print
ahd_sgseqbcnt_print
ahd_slvspltoutattr0_print
ahd_slvspltoutattr1_print
ahd_slvspltoutattr2_print
ahd_sgspltstat0_print
ahd_sgspltstat1_print
ahd_sfunct_print
ahd_df0pcistat_print
ahd_reg0_print
ahd_df1pcistat_print
ahd_sgpcistat_print
ahd_reg1_print
ahd_cmcpcistat_print
ahd_ovlypcistat_print
ahd_reg_isr_print
ahd_msipcistat_print
ahd_targpcistat_print
ahd_data_count_odd_print
ahd_scbptr_print
ahd_ccscbacnt_print
ahd_scbautoptr_print
ahd_ccsgaddr_print
ahd_ccscbadr_bk_print
ahd_ccscbaddr_print
ahd_cmc_rambist_print
ahd_ccsgram_print
ahd_flexadr_print
ahd_ccscbram_print
ahd_flexcnt_print
ahd_flexdmastat_print
ahd_flexdata_print
ahd_brddat_print
ahd_brdctl_print
ahd_seeadr_print
ahd_seedat_print
ahd_seectl_print
ahd_seestat_print
ahd_scbcnt_print
ahd_dfwaddr_print
ahd_dspfltrctl_print
ahd_dspdatactl_print
ahd_dfraddr_print
ahd_dspreqctl_print
ahd_dspackctl_print
ahd_dfdat_print
ahd_dspselect_print
ahd_wrtbiasctl_print
ahd_rcvrbiosctl_print
ahd_wrtbiascalc_print
ahd_rcvrbiascalc_print
ahd_dfptrs_print
ahd_skewcalc_print
ahd_dfbkptr_print
ahd_dfdbctl_print
ahd_dfscnt_print
ahd_dfbcnt_print
ahd_ovlyaddr_print
ahd_seqctl1_print
ahd_flags_print
ahd_seqram_print
ahd_prgmcnt_print
ahd_accum_print
ahd_sindex_print
ahd_dindex_print
ahd_brkaddr0_print
ahd_brkaddr1_print
ahd_allones_print
ahd_allzeros_print
ahd_none_print
ahd_sindir_print
ahd_dindir_print
ahd_function1_print
ahd_stack_print
ahd_intvec1_addr_print
ahd_curaddr_print
ahd_lastaddr_print
ahd_intvec2_addr_print
ahd_longjmp_addr_print
ahd_accum_save_print
ahd_waiting_scb_tails_print
ahd_ahd_pci_config_base_print
ahd_sram_base_print
ahd_waiting_tid_head_print
ahd_waiting_tid_tail_print
ahd_next_queued_scb_addr_print
ahd_complete_scb_head_print
ahd_complete_scb_dmainprog_head_print
ahd_complete_dma_scb_head_print
ahd_complete_dma_scb_tail_print
ahd_complete_on_qfreeze_head_print
ahd_msg_out_print
ahd_dmaparams_print
ahd_saved_scsiid_print
ahd_saved_lun_print
ahd_qoutfifo_entry_valid_tag_print
ahd_kernel_tqinpos_print
ahd_tqinpos_print
ahd_shared_data_addr_print
ahd_qoutfifo_next_addr_print
ahd_arg_1_print
ahd_arg_2_print
ahd_last_msg_print
ahd_scsiseq_template_print
ahd_initiator_tag_print
ahd_allocfifo_scbptr_print
ahd_int_coalescing_timer_print
ahd_int_coalescing_maxcmds_print
ahd_int_coalescing_mincmds_print
ahd_cmds_pending_print
ahd_int_coalescing_cmdcount_print
ahd_local_hs_mailbox_print
ahd_cmdsize_table_print
ahd_scb_base_print
ahd_scb_residual_datacnt_print
ahd_scb_residual_sgptr_print
ahd_scb_scsi_status_print
ahd_scb_target_phases_print
ahd_scb_target_data_dir_print
ahd_scb_target_itag_print
ahd_scb_sense_busaddr_print
ahd_scb_tag_print
ahd_scb_lun_print
ahd_scb_task_attribute_print
ahd_scb_cdb_len_print
ahd_scb_task_management_print
ahd_scb_dataptr_print
ahd_scb_datacnt_print
ahd_scb_sgptr_print
ahd_scb_busaddr_print
ahd_scb_next_print
ahd_scb_next2_print
ahd_scb_spare_print
ahd_scb_disconnected_lists_print
" | while read name; do
    grep "$name" -rc .
done | grep -vF ':0'

I got the following output:
./aic79xx_reg.h_shipped:2
./aic79xx_reg_print.c_shipped:1
./aic79xx_reg.h_shipped:2
./aic79xx_reg_print.c_shipped:1
...
...

which means that these functions are defined but never referenced.

(Yes, patch similarly ifdefs out function definitions in aic79xx_reg_print.c_shipped)


> And in either case the *_shipped files are in fact
> autogenerated by aic assembler, so we need to fix that
> one, too.

I tried this approach. But running aic assembler doesn't seem
to regenerate _shipped files.

I built kernel with the following options and none of the *_shipped
files change (in fact, no source file changes):

# CONFIG_PREVENT_FIRMWARE_BUILD is not set
CONFIG_SCSI_AIC7XXX=y
CONFIG_AIC7XXX_CMDS_PER_DEVICE=8
CONFIG_AIC7XXX_RESET_DELAY_MS=15000
CONFIG_AIC7XXX_BUILD_FIRMWARE=y   <============
CONFIG_AIC7XXX_DEBUG_ENABLE=y
CONFIG_AIC7XXX_DEBUG_MASK=0
CONFIG_AIC7XXX_REG_PRETTY_PRINT=y
# CONFIG_SCSI_AIC7XXX_OLD is not set
CONFIG_SCSI_AIC79XX=y
CONFIG_AIC79XX_CMDS_PER_DEVICE=8
CONFIG_AIC79XX_RESET_DELAY_MS=15000
CONFIG_AIC79XX_BUILD_FIRMWARE=y  <============
CONFIG_AIC79XX_DEBUG_ENABLE=y
CONFIG_AIC79XX_DEBUG_MASK=0
CONFIG_AIC79XX_REG_PRETTY_PRINT=y
CONFIG_SCSI_AIC94XX=m
CONFIG_AIC94XX_DEBUG=y

(full .config is attached)

I also grepped through the directory and found no rules in Makefiles
which regenerate _shipped files.

If you know how to regenerate them, please let me know.

Any other objections to the patch?
--
vda

View attachment ".config" of type "text/plain" (36184 bytes)

Powered by blists - more mailing lists